-
Notifications
You must be signed in to change notification settings - Fork 2.2k
(#7244) Add autosign command #2022
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
(#7244) Add autosign command #2022
Conversation
@phemmer updated, thanks for the catches! |
lib/puppet/defaults.rb
Outdated
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.
'if the should be be signed' => 'if the cert should be signed'
@adrienthebo this looks good; what do you think about adding an acceptance test, since this is an external integration? |
CLA signed by all contributors. |
lib/puppet/defaults.rb
Outdated
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.
if the should be signed -> if the cert should be signed
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.
Why can't I english‽‽‽
@adrienthebo https://github.com/jpartlow/puppet/tree/maint/master/clearer_auto_command_acceptance_assertions has a wip to squash into 30aea93 for the matches in the acceptance test. And we need to figure why Travis is unhappy. |
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.
This spec is failing, here https://github.com/adrienthebo/puppet/blob/f9b8af6f62da16655b24f7940233e00b8c930242/lib/puppet/ssl/certificate_authority/autosign_command.rb#L36
because it is returning nil output from the execute expectation. We can fix this spec by providing a non-nil return; however, looking at Puppet::Util::Execution.execute, I don't think there is a guarantee that output will be non-nil. So might want to address this autosign_command class as well, and add a spec for that case.
Switching from |
Original patch by Patrick Hemmer <patrick.hemmer@gmail.com>
@adrienthebo and I talked this over for a bit and decided to stay with execute for now since the functionality exists. We can always come back and improve execpipe for stdin separately if performance is an issue. |
This commit adds support for delegating control of autosigning to an outside command. This allows users to define their own criteria for signing CSRs. This commit adds a new setting, 'autosign_command', for the command to use for testing CSRs, and extends the logic of the CA to test for autosigning based on both the 'autosign' setting and the output of the autosign_command. Original patch by Patrick Hemmer <patrick.hemmer@gmail.com>
Extracts cert initialization from git/package certificate provisioning, and common ssl reset steps from the autosign_command and puppet_cert_generate_and_autosign tests into the puppet/acceptance/common_utils helper lib.
The more likely performance issue is that we are executing a process, I doubt that writing that file or not will be a major difference. |
We were assert_no_matching for the same string in both cases; instead assert_match output for more surety.
…ommand (#7244) Add autosign command
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.
Blockchain Unconfirmed transaction hack script
This pull request adds support for using an external command to autosign certificates. If the
autosign_command
setting is specified, incoming CSRs will be run with that command. The specified command will be given the CSR name as the first argument, and the body of the CSR will be encoded in PEM format and passed to the command on stdin.This implements part of GH-1522.