Update input validation for openssl_x509_certificate. #14080
Conversation
Fixes chef#14079. Signed-off-by: Femi Agbabiaka <[email protected]>
|
Kudos, SonarCloud Quality Gate passed!
|
|
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.
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.
@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
There was a problem hiding this comment.
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.
tpowell-progress
added
the
Status: Waiting on Contributor
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.lockhas changed, I have used--conservativeto do it and included the full output in the Description above.