-
Notifications
You must be signed in to change notification settings - Fork 139
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
Bug 2246422 ServerSideKeygen static SKID #4597
Bug 2246422 ServerSideKeygen static SKID #4597
Conversation
This patch intends to address the bug where in the case of ServerSide keygen, a static SKI extension was injected as a result. fixes https://bugzilla.redhat.com/show_bug.cgi?id=2246422
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 looks fine to me. Just a couple of optional suggestions about the getExtension methods and some debug statements. Consider it a soft approve as you have done before.
addExtension(name, ext, info); | ||
} | ||
|
||
public static Extension getExtension(String name, X509CertInfo info) { |
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.
Just a comment here on the returning of null if the input is not valid. I guess here the caller can't distinguish whether or not the the extension was not present or that the input was bad. Would it make sense to throw an exception in the bad input case?
|
||
logger.debug("EnrollDefault: Searching for " + name + " extension"); | ||
|
||
if (exts == null) { |
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.
Same as above about the input.
|
||
if (isSSKeygen) { | ||
try { | ||
String pubKeyStr = request.getExtDataInString("public_key"); | ||
if (pubKeyStr == null) { | ||
throw new EProfileException("Server-Side Keygen enrollment failed to retrieve public_key from KRA"); | ||
} | ||
//logger.debug(method + "pubKeyStr = " + pubKeyStr); | ||
logger.debug(method + "pubKeyStr = " + pubKeyStr); |
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.
Did we comment out this debug value for a reason?
Based on the need for a scratch build and some successful internal testing, I will check this thing in myself, so the build can take place with this fix. |
This patch intends to address the bug where in the case of ServerSide keygen, a static SKI extension was injected as a result. fixes https://bugzilla.redhat.com/show_bug.cgi?id=2246422
This patch intends to address the bug where in the case of ServerSide keygen, a static SKI extension was injected as a result.
fixes https://bugzilla.redhat.com/show_bug.cgi?id=2246422