-
Notifications
You must be signed in to change notification settings - Fork 6.1k
8255409: Support the new C_GetInterfaceList, C_GetInterface, and C_SessionCancel APIs in PKCS#11 v3.0 #6655
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
👋 Welcome back valeriep! A progress list of the required criteria for merging this PR into |
@valeriepeng The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
@@ -409,65 +415,89 @@ private void parse() throws IOException { | |||
throw excToken("Unexpected token:"); | |||
} | |||
String word = st.sval; | |||
if (word.equals("name")) { | |||
switch (word) { |
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.
Since every case has a break it's probably better to use the enhanced switch (case "x" -> ...;
). It's safer and also saves quite some lines. An IDE can help you with the conversion.
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.
Sure, I can do that. I tend to use the "->" for switch with less items.
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.
According to JEP 361, the preferred style is to write the statement (if there is only a single line) on the same line as the case label, so you save another one line.
@@ -401,8 +401,27 @@ private void implInit(int opmode, Key key, byte[] iv, int tagLen, | |||
} | |||
|
|||
private void cancelOperation() { | |||
// cancel operation by finishing it; avoid killSession as some | |||
// hardware vendors may require re-login |
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.
The new cancelOperation()
methods seems identical everywhere. Is it possible to consolidate it to a helper method like trySessionCancel(token, session, flags)
? It can return true if canceled successfully, false if needs a fallback, and can still throw a ProviderException
.
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.
I assume you mean the if-() block of code? I can move the code into a helper method inside the P11Util class.
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.
Yes, just keep duplicated lines as few as possible.
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.
Should the NSS supporting 3.0 get added to the changeset for testing?
// cancel operation by finishing it; avoid killSession as some | ||
// hardware vendors may require re-login | ||
if (token.p11.getVersion().major == 3) { | ||
long flags = (mode == M_SIGN? CKF_SIGN : CKF_VERIFY); |
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.
I think this is a syntax nit with no space between M_SIGN and '?'
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.
Ok, will add the space.
token.ensureValid(); | ||
|
||
if (token.p11.getVersion().major == 3) { | ||
long flags = (opmode == Cipher.ENCRYPT_MODE? CKF_ENCRYPT : |
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.
I think this is a syntax nit with no space between MODE and '?'
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.
Ok.
// cancel operation by finishing it; avoid killSession as some | ||
// hardware vendors may require re-login | ||
if (token.p11.getVersion().major == 3) { | ||
long flags = (encrypt? CKF_ENCRYPT : CKF_DECRYPT); |
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.
I think this is a syntax nit with no space between "encrypt" and '?'
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.
Ok.
// hardware vendors may require re-login | ||
token.ensureValid(); | ||
if (token.p11.getVersion().major == 3) { | ||
long flags = (encrypt? CKF_ENCRYPT : CKF_DECRYPT); |
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.
I think this is a syntax nit with no space between "encrypt" and '?'
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.
Ok.
// hardware vendors may require re-login | ||
|
||
if (token.p11.getVersion().major == 3) { | ||
long flags = (mode == M_SIGN? CKF_SIGN : CKF_VERIFY); |
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.
M_SIGN ? ...
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.
Ok. BTW, is there a central place for these syntax? I usually just pick up the convention used in the same file.
I updated my comments because I neglected to read your initial message and went straight to the code review |
Recall there was a bug filed for updating the artifactory NSS version. There was some window build issue, will follow up with the SQE RE again. |
if (token.p11.getVersion().major == 3) { | ||
try { | ||
token.p11.C_SessionCancel(session.id(), flags); | ||
status = true; |
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.
Or you can just return true
here and return false
at the end.
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.
Sure, I like this better.
} | ||
|
||
// only used by cancelOperation(); cancel by finishing operations | ||
// avoid killSession as some hardware vendors may require re-login |
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.
Since this method is only called once, maybe we can inline it back inside cancelOperation()
?
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.
Ok~
@@ -186,11 +203,14 @@ public static synchronized PKCS11 getInstance(String pkcs11ModulePath, | |||
* native part. | |||
* | |||
* @param pkcs11ModulePath The PKCS#11 library path. | |||
* @param functionList the method name for retrieving the PKCS#11 | |||
* function list; maybe null if not set in config file |
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.
s/maybe/may be/.
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.
Sure. Will fix both of the "maybe" s in this file.
@@ -351,7 +378,7 @@ JNIEXPORT void JNICALL Java_sun_security_pkcs11_wrapper_PKCS11_C_1SetOperationSt | |||
|
|||
free(ckpState); | |||
|
|||
if (ckAssertReturnValueOK(env, rv) != CK_ASSERT_OK) { return; } | |||
ckAssertReturnValueOK(env, rv); |
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.
Are you going to apply this change to other places? I see 2 new functions still using the old style.
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.
Sure, I can change the other 2 places.
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.
After my most recent comments are addressed, I'm fine with the change
ckPinLength); | ||
free(ckpPinArray); | ||
|
||
if (ckAssertReturnValueOK(env, rv) != CK_ASSERT_OK) { return; } |
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.
Given you changed line 381 to not be in a condition, I would assume this could be the same.
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.
Yes, will change.
|
||
free(ckpPinArray); | ||
free(ckpUsername); | ||
|
||
if (ckAssertReturnValueOK(env, rv) != CK_ASSERT_OK) { return; } |
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.
Conditional again?
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.
Yes, will change.
@valeriepeng This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be:
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 170 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
/integrate |
Going to push as commit 83e6a4c.
Your commit was automatically rebased without conflicts. |
@valeriepeng Pushed as commit 83e6a4c. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
PKCS#11 v3.0 adds the support for several new APIs. For this particular RFE, it enhances SunPKCS11 provider to load PKCS#11 provider by first trying the C_GetInterface (new in 3.0) before the C_GetFunctionList assuming not explicitly specified in config. In addition, PKCS#11 v3.0 defines a new API for cancelling session operations, so I've also updated various classes to call this new API if the PKCS#11 library version is 3.0. Otherwise, these classes will try to cancel by finishing off current operations as before. The support for the new C_LoginUser() has not been tested, so I commented it out for now. Given the current release schedule, support for other new PKCS#11 APIs (such as message-based ones and parameters structure) and options for C_GetInterface (if needed) will be handled later.
I validated the current changes against different NSS releases (supports PKCS#11 v2.40 and v3..0 respectively) with existing regression tests.
Thanks,
Valerie
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/6655/head:pull/6655
$ git checkout pull/6655
Update a local copy of the PR:
$ git checkout pull/6655
$ git pull https://git.openjdk.java.net/jdk pull/6655/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 6655
View PR using the GUI difftool:
$ git pr show -t 6655
Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/6655.diff