-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Update input validation for openssl_x509_certificate. #14080
base: main
Are you sure you want to change the base?
Update input validation for openssl_x509_certificate. #14080
Conversation
Fixes chef#14079. Signed-off-by: Femi Agbabiaka <femi@femiagbabiaka.xyz>
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
I'm not 100% sure on how to resolve the last three bullet points in the checklist, and could use some assistance/advice in doing so. I assume that the documentation is generated by updating the property definition? |
@@ -212,13 +212,18 @@ def request | |||
end | |||
|
|||
def subject | |||
if new_resource.common_name.nil? && new_resource.subject_alt_name.empty? | |||
Chef::Log.fatal("Neither common_name nor subject_alt_name specified, one is required.") | |||
raise "Neither common_name nor subject_alt_name specified, one is required." |
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.
While this may be the correct way of doing things, I have concerns that this could break people by surprise. Can we do a deprecation first?
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.
@tpowell-progress that makes sense, thanks. I'm new to deprecation warnings in chef resources, is this a good pattern to follow?
chef/lib/chef/resource/archive_file.rb
Line 137 in d06dc07
def define_resource_requirements |
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.
Looking around, I think that pattern makes sense, provided that you can accomplish the deprecation warning without getting too messy. If for some reason affixing the deprecation warning doesn't look clean, we can dig deeper, but I'd recommend just pushing a reasonable try at it and we can see if makes sense.
Fixes #14079.
Description
TL;DR, as I understand it, you must have one of common_name or a subject_alt_name entry. If you have a common_name entry, it must match the subject_alt_name entry. But common_name is not required.
Currently, the resource implicitly requires common_name to be specified: https://github.com/chef/chef/blob/main/lib/chef/resource/openssl_x509_certificate.rb#L221. Implicit because common_name isn't marked as required: https://github.com/chef/chef/blob/main/lib/chef/resource/openssl_x509_certificate.rb#L101.
I believe the correct solution is to leave the common_name and subject_alt_name property definitions as-is, add an unless check on the nilness of common_name, and require that one of subject_alt_name or common_name be set.
Related Issue
#14079
Types of changes
Checklist:
Gemfile.lock
has changed, I have used--conservative
to do it and included the full output in the Description above.