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

Merged
luispresuelVenafi merged 2 commits intoVenafi:masterfrom
inteon:fix_ec
Oct 18, 2022
Merged

Conversation

@inteon
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

luispresuelVenafi commented Oct 13, 2022

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
Copy Markdown
Contributor Author

inteon commented Oct 18, 2022

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

@luispresuelVenafi
Copy link
Copy Markdown
Contributor

luispresuelVenafi commented Oct 18, 2022

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
Copy Markdown
Contributor Author

inteon commented Oct 18, 2022

@luispresuelVenafi done

@luispresuelVenafi
Copy link
Copy Markdown
Contributor

luispresuelVenafi commented Oct 18, 2022

@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
Copy Markdown
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