-
Notifications
You must be signed in to change notification settings - Fork 64
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.
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
Add missing RSA 3072 bit key size #536
Conversation
Hi @inteon , |
pkg/venafi/tpp/connector_test.go
Outdated
@@ -1981,7 +1981,7 @@ func TestReadPolicyConfiguration(t *testing.T) { | |||
[]string{".*"}, | |||
[]string{".*"}, | |||
[]endpoint.AllowedKeyConfiguration{ | |||
{certificate.KeyTypeRSA, certificate.AllSupportedKeySizes(), nil}, | |||
{certificate.KeyTypeRSA, []int{1024, 2048, 3072, 4096, 8192}, nil}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you revert this, as it was better set as it was before. Also I have question. Is this size allowed in TPP 22.3? If not, we will need to upgrade, since running test got the following:
get: {SubjectCNRegexes:[^([\p{L}\p{N}-*]+\.)*vfidev\.com$ ^([\p{L}\p{N}-*]+\.)*vfidev\.net$ ^([\p{L}\p{N}-*]+\.)*vfide\.org$] SubjectORegexes:[^Venafi Inc\.$] SubjectOURegexes:[^Integration$] SubjectSTRegexes:[^Utah$] SubjectLRegexes:[^Salt Lake$] SubjectCRegexes:[^US$] AllowedKeyConfigurations:[{KeyType:0 KeySizes:[2048 3072 4096 8192] KeyCurves:[]}] DnsSanRegExs:[^([\p{L}\p{N}-*]+\.)*vfidev\.com$ ^([\p{L}\p{N}-*]+\.)*vfidev\.net$ ^([\p{L}\p{N}-*]+\.)*vfide\.org$] IpSanRegExs:[.*] EmailSanRegExs:[.*] UriSanRegExs:[.*] UpnSanRegExs:[.*] AllowWildcards:true AllowKeyReuse:true}
expect: {SubjectCNRegexes:[^([\p{L}\p{N}-*]+\.)*vfidev\.com$ ^([\p{L}\p{N}-*]+\.)*vfidev\.net$ ^([\p{L}\p{N}-*]+\.)*vfide\.org$] SubjectORegexes:[^Venafi Inc\.$] SubjectOURegexes:[^Integration$] SubjectSTRegexes:[^Utah$] SubjectLRegexes:[^Salt Lake$] SubjectCRegexes:[^US$] AllowedKeyConfigurations:[{KeyType:0 KeySizes:[2048 4096 8192] KeyCurves:[]}] DnsSanRegExs:[^([\p{L}\p{N}-*]+\.)*vfidev\.com$ ^([\p{L}\p{N}-*]+\.)*vfidev\.net$ ^([\p{L}\p{N}-*]+\.)*vfide\.org$] IpSanRegExs:[.*] EmailSanRegExs:[.*] UriSanRegExs:[.*] UpnSanRegExs:[.*] AllowWildcards:true AllowKeyReuse:true}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this keysize was already supported in 2021: https://github.com/Venafi/vcert/pull/164/files#diff-f30f61745dc1d11a1a2455b36477d3840e36d4b66e3f6c32854db74c9efd51faR13
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reverted the change and fixed the failing test.
769cb1a
to
330b56a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's pending to fix the following:
[2024-11-05T19:23:20.684Z] === RUN TestConvertServerPolicyToInternalPolicy
[2024-11-05T19:23:20.684Z] tpp_test.go:315: bad key lengths
[2024-11-05T19:23:20.684Z] --- FAIL: TestConvertServerPolicyToInternalPolicy (0.00s)
Thank you for letting me know. I updated the test such that it succeeds. I think that CI testing would be very helpful for this project, it would drastically reduce the time necessary to find and fix these test issues. |
e32b9da
to
5487cd3
Compare
Hi @inteon your changes looked good in our pipeline. I tried to merge but I couldn't since your commits aren't signed (just noticed that, wish I could tell you sooner): It is a requirement to merge changes into our Open Source repositories, for security purposes: You can learn more about signing commits here: |
Signed-off-by: Tim Ramlot <[email protected]>
Signed-off-by: Tim Ramlot <[email protected]>
Signed-off-by: Tim Ramlot <[email protected]>
5487cd3
to
79d4b5f
Compare
@luispresuelVenafi All commits are signed now. |
This causes the
.ValidateCertificateRequest()
validation function to fail for 3072 bit RSA keys.