Skip to content

(SERVER-119) Properly sign puppet extension values #51

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

Conversation

dankreek
Copy link

No description provided.

@dankreek dankreek closed this Jan 26, 2015
@dankreek dankreek reopened this Jan 27, 2015
@dankreek dankreek force-pushed the SERVER-119/sign-puppet-extensions-properly branch from 4398a28 to dcbaf8f Compare January 27, 2015 18:04
@jeffmccune
Copy link
Contributor

It'd be great if the commit message for 4943604 contained at least the following information: The behavior of the system without the patch applied, why this is a problem, and how the change resolves the problem.

@@ -189,28 +196,43 @@ public static X509Certificate signCertificate(String issuerDn,
String subjectDn,
PublicKey subjectPublicKey,
List<Map<String, Object>> extensions)
throws IOException, OperatorCreationException, CertificateException, NoSuchAlgorithmException, SignatureException, InvalidKeyException {
X509V3CertificateGenerator certGen = new X509V3CertificateGenerator();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh sweet, so we were able to get rid of using this deprecated class? Finally. Looks like we can get rid of the import statement for this class (surprised IDEA didn't take care of that).

@MSLilah
Copy link
Contributor

MSLilah commented Feb 3, 2015

I'm 👍 on this barring @jeffmccune's comment about the commit message. My knowledge of this is limited, however, so someone else should definitely take a look at it.

@jeffmccune
Copy link
Contributor

I've got no other comments either, I'm 👍 except the housekeeping comment.

Looks good.

@dankreek
Copy link
Author

dankreek commented Feb 3, 2015

If ya'lls ready to merge, let me know and I can amend that commit message before merge

Justin May added 3 commits February 3, 2015 14:56
Currently, this library signs all custom extensions as binary data wrapped
in a DER Octet String, which is the same way the Ruby Puppet Master does
it. According to RFC 5280, the values of all X.509 extensions must be a
valid DER encoded structure contained within a DER Octet String, so the
current method is technically incorrect.

The Bouncy Castle library assumes adherence to this standard, so trying
to exactly mimic the way MRI Puppet signs trusted facts has proven to be
very unwieldy and brittle.

Other extensions which contain a simple string as a value will use a
UTF8String type and then wrap it inside of a DER Octet String.
Did this just to maintain uniformity in our test fixtures.
@dankreek dankreek force-pushed the SERVER-119/sign-puppet-extensions-properly branch from dcbaf8f to 23ce8ef Compare February 3, 2015 23:00
@jeffmccune jeffmccune merged commit 23ce8ef into puppetlabs:master Feb 4, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants