Skip to content

Adds EC Curves for VaaS's certificatePolicies utils certificate's method "toPolicy" and connector's "buildPolicySpecification" function, as well as adds support for ED25519 curve #261

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Oct 18, 2022
Merged

Adds EC Curves for VaaS's certificatePolicies utils certificate's method "toPolicy" and connector's "buildPolicySpecification" function, as well as adds support for ED25519 curve #261

merged 2 commits into from
Oct 18, 2022

Conversation

inteon
Copy link
Contributor

@inteon inteon commented Sep 29, 2022 •
edited
Loading

This caused the ReadZoneConfiguration function to return an empty keyCurves array for VaaS.

@maelvls
Copy link
Contributor

maelvls commented Sep 29, 2022 •
edited
Loading

How can I reproduce the bug? Is there a way to reproduce it with a few curl commands? Which version of TPP are you using?

@tr1ck3r
Copy link
Member

tr1ck3r commented Sep 29, 2022

Do we need to account for VaaS's support for the ED25519 curve and RSA 3072 in https://github.com/Venafi/vcert/blob/master/pkg/certificate/certificate.go? TPP shouldn't have any problems with RSA 3072 even though the UI doesn't support it but TPP doesn't support ED25519 for certificates as far as I'm aware.

@inteon
Copy link
Contributor Author

inteon commented Sep 29, 2022 •
edited
Loading

Do we need to account for VaaS's support for the ED25519 curve and RSA 3072 in https://github.com/Venafi/vcert/blob/master/pkg/certificate/certificate.go? TPP shouldn't have any problems with RSA 3072 even though the UI doesn't support it but TPP doesn't support ED25519 for certificates as far as I'm aware.

My code is using

vcert/pkg/certificate/certificate.go

Lines 58 to 71 in a3d4338

func (ec *EllipticCurve) Set(value string) error {
switch strings.ToLower(value) {
case "p521", "p-521":
*ec = EllipticCurveP521
case "p384", "p-384":
*ec = EllipticCurveP384
case "p256", "p-256":
*ec = EllipticCurveP256
default:
*ec = EllipticCurveDefault
}
return nil
}
, so you probably want to add support for the ED25519 curve to this function too.
Maybe a bit out-of-scope for this PR?

@luispresuelVenafi
Copy link
Contributor

I already added the missing test cases and fixed another bug regarding getting EC curve when using GetPolicy method on the side of VaaS (Cloud) connector. I needed to add the ED25519 curve in order to work correctly with our issuing template. I believe now we can merge this PR. @marcos-albornoz , @rvelaVenafi , @EduardoVV, could you give your review and approval (if it applies)?

@luispresuelVenafi luispresuelVenafi changed the title VaaS toPolicy should also copy the EC keyCurves Adds EC Curves for VaaS certificateTemplate's toPolicy and buildPolicySpecification function, as well as adds support for ED25519 curve Oct 13, 2022
@luispresuelVenafi luispresuelVenafi changed the title Adds EC Curves for VaaS certificateTemplate's toPolicy and buildPolicySpecification function, as well as adds support for ED25519 curve Adds EC Curves for VaaS's certificatePolicies utils certificate's method "toPolicy" and connector's "buildPolicySpecification" function, as well as adds support for ED25519 curve Oct 13, 2022
@inteon inteon force-pushed the fix_ec branch 2 times, most recently from cc810c7 to 39e612d Compare October 18, 2022 07:37
@inteon
Copy link
Contributor Author

inteon commented Oct 18, 2022

@luispresuelVenafi I rebased the PR, should be ready to merge.

@luispresuelVenafi
Copy link
Contributor

Hi @inteon , could you sign your commit? Currently, the missing sign of the commit is blocking the merging of this PR (now signed commits is a requirement to contribute into our projects).

@inteon
Copy link
Contributor Author

inteon commented Oct 18, 2022

@luispresuelVenafi done

@luispresuelVenafi
Copy link
Contributor

@inteon it still marks it as unverified commit

inteon added 2 commits October 18, 2022 19:06
@inteon
VaaS toPolicy should also copy the keyCurves
f88cb7b 
Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>
@inteon
Adds missing logic to handle recommended settings EC Curves values wh…
c6f2722 
…en getting policy specificiation. Adds test accordingly and test for getting EC Curves values when calling ReadZoneConfiguration function that calls toPolicy

Co-authored-by: Luis Presuel <luis.presuel@venafi.com>
Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>
@inteon
Copy link
Contributor Author

inteon commented Oct 18, 2022 •
edited
Loading

@luispresuelVenafi sorry for the confusion, I thought you were talking about GH Sign-off instead of signing commits. I signed both commits now.

@luispresuelVenafi luispresuelVenafi merged commit 43aa507 into Venafi:master Oct 18, 2022
@inteon inteon deleted the fix_ec branch October 18, 2022 17:40
@maelvls maelvls mentioned this pull request Jan 4, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rvelaVenafi rvelaVenafi rvelaVenafi approved these changes

@marcos-albornoz marcos-albornoz marcos-albornoz approved these changes

@EduardoVV EduardoVV EduardoVV approved these changes

@luispresuelVenafi luispresuelVenafi Awaiting requested review from luispresuelVenafi luispresuelVenafi is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@inteon @maelvls @tr1ck3r @luispresuelVenafi @rvelaVenafi @marcos-albornoz @EduardoVV