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
Complete deprecation annotations already started #26
Complete deprecation annotations already started #26
Conversation
Due the size I recommend reviewing this after the other simpler pull requests have been handled. |
@emaldona Thanks for the other patches! They have been merged. :) Any chance you'll be around to update this one today? |
Yes, I'll work on it Today. It will take me a while to update as this one is a lot bigger. I'll be online. |
Two lines with "@deprecated" annotation but no reason given. Lines 405 and 413 in the patch, line 399 ponts to org/mozilla/jss/netscape/security/provider/DSA.java. Reason for deprecation given at https://docs.oracle.com/javase/7/docs/api/java/security/SignatureSpi.html where it states |
The Oracle site gives a reason for by engineSetParameter(String param, Object value) deprecation but none for the getParameter(Sring key) one. Explanation for engineGetParameter(String param) |
Sorry, this latest version was generated incorrectly. It includes the stuff from the one for issue #30. The previous version was fine. @cipherboy I might need some help fixing this. |
Okay, so I see a few deprecations changed... But tell me what I'm supposed to be looking for?
So in > ./org/mozilla/jss/pkcs11/PK11Cipher.java:102: warning: [deprecation] isPadded() in EncryptionAlgorithm has been deprecated
> ./org/mozilla/jss/pkcs11/PK11Cipher.java:105: warning: [deprecation] isPadded() in EncryptionAlgorithm has been deprecated
> ./org/mozilla/jss/pkcs11/PK11Cipher.java:127: warning: [deprecation] isPadded() in EncryptionAlgorithm has been deprecated
> ./org/mozilla/jss/pkcs11/PK11Cipher.java:130: warning: [deprecation] isPadded() in EncryptionAlgorithm has been deprecated
> ./org/mozilla/jss/pkcs11/PK11Cipher.java:165: warning: [deprecation] isPadded() in EncryptionAlgorithm has been deprecated
> ./org/mozilla/jss/pkcs11/PK11Cipher.java:192: warning: [deprecation] isPadded() in EncryptionAlgorithm has been deprecated This calls However, in your branch you add a jss/org/mozilla/jss/crypto/EncryptionAlgorithm.java Lines 276 to 283 in ac7ae24
Why? Don't we want to propagate that Perhaps @jmagne or @edewata has some idea on this. :) Then there's this change in your branch: < ./org/mozilla/jss/pkcs11/CipherContextProxy.java:16: warning: [deprecation] finalize() in NativeProxy has been deprecated
< ./org/mozilla/jss/pkcs11/CipherContextProxy.java:17: warning: [deprecation] finalize() in NativeProxy has been deprecated
< ./org/mozilla/jss/pkcs11/KeyProxy.java:12: warning: [deprecation] finalize() in NativeProxy has been deprecated
< ./org/mozilla/jss/pkcs11/KeyProxy.java:13: warning: [deprecation] finalize() in NativeProxy has been deprecated
< ./org/mozilla/jss/pkcs11/ModuleProxy.java:16: warning: [deprecation] finalize() in NativeProxy has been deprecated
< ./org/mozilla/jss/pkcs11/ModuleProxy.java:17: warning: [deprecation] finalize() in NativeProxy has been deprecated It appears that it is using You make the change to add jss/org/mozilla/jss/util/NativeProxy.java Lines 76 to 97 in ac7ae24
This looks like This looks like we could deprecate our overriding of The line number changed on this: < ./org/mozilla/jss/pkcs11/PK11PrivKey.java:45: warning: [deprecation] getUniqueID() in PrivateKey has been deprecated
---
> ./org/mozilla/jss/pkcs11/PK11PrivKey.java:43: warning: [deprecation] getUniqueID() in PrivateKey has been deprecated You added the deprecations around it: jss/org/mozilla/jss/pkcs11/PK11PrivKey.java Lines 36 to 45 in ac7ae24
But I'm not sure they're in the correct place? Anyhow, perhaps that'll be interesting feedback. Any thoughts to aid my understanding? |
Please see my latest two posting to https://pagure.io/jss/issue/19 |
In the attachment https://pagure.io/jss/issue/19#comment-533900 There are 45 warning before and after but there are differences
9 warnings that showed up after the patch:
Notice that they all refer to org.mozilla.jss.util.NativeProxy; which these classes import and that has been annotated there. So I disdn't bother to annotate the other ones. The rest are identical aside from line numbers changes to be expected.
Yes, I do in many places. From what I have read '@Deprecates' is a javadoc is for javadoc whereas '@deprecated' is for the compiler.
Yes, we should consider this. This pull request is intended to annotate only and make life easier for the maintainer who will be working on addressing these on https://pagure.io/jss/issue/17 many months from now.
Worth considering for https://pagure.io/jss/issue/17
Applying the patch causes code movement.
I hope the latest attachments to issue #19 help a bit. Also, using meld to compare, thought not automation friendly, helped see things better. Will be online so we can discuss this stuff and try out things. |
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.
Okay, a few suggestions on improving this PR and a question. Thanks!
@@ -276,6 +277,7 @@ public int getBlockSize() { | |||
* @return <code>true</code> if this algorithm performs padding. | |||
* @deprecated Call <tt>getPaddingType()</tt> instead. |
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.
Okay, I'm going to recommend this be un-deprecated. It looks like a useful function and I'm inclined to keep it, especially as it is small and the cost of maintaining it should be minimal.
@@ -276,6 +277,7 @@ public int getBlockSize() { | |||
* @return <code>true</code> if this algorithm performs padding. | |||
* @deprecated Call <tt>getPaddingType()</tt> instead. | |||
*/ | |||
@Deprecated |
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.
Drop this line as well.
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.
Drop the 289 line but still keep the other one at 278?
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 it is going to be un-deprecated, I'd drop both this line and line 278 (from the first comment).
@@ -37,7 +37,9 @@ public native void verifyKeyIsOnToken(PK11Token token) | |||
* Returns a new CryptoToken where this key resides. | |||
* | |||
* @return The PK11Token that owns this key. | |||
* @deprecated getUniqueID() in PrivateKey has been deprecated |
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 is in the wrong place. It should be in a new comment and modify getUniqueID()
; as it currently is, it modifies getOwningToken()
.
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.
Per conversation with @edewata, I think we should un-deprecate these as well.
*/ | ||
@Deprecated |
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 with this, move it down two lines.
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.
Actually, don't move it, just remove it. :)
* SSL3_RSA_WITH_DES_CBC_SHA | ||
* SSL3_RSA_WITH_NULL_MD5 | ||
*/ | ||
|
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.
Does this need an @Deprecated
annotation?
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.
Many do but I didn't because it was getting messy and thought some are needed for reasons of compatibility so I opted to just list them in a conventional comment.side note: I think I saw somewhere that jss now plays well the system wide crypto policy and will likely prevent usage of insecure cipher suites. These two seem to be the worst. Are you suggesting me to add the @Deprecatedannotation to only those two or all of them?
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 know nss
supports system-wide crypto policy, but I'm not sure about jss
. I've been meaning to take a look at that; I think the first step is creating a basic client/server, deploying that to a system, and then automating testing of various cipher suite configurations.
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.
Okay, please un-deprecate the getUniqueID()
changes as well.
@@ -37,6 +37,7 @@ | |||
* another way, such as a function that directly matches a cert and | |||
* key. | |||
*/ | |||
@Deprecated |
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.
Per conversation with @edewata, I think we should un-deprecate these as well.
@@ -22,6 +22,7 @@ | |||
* another way, such as a function that directly matches a cert and | |||
* key. | |||
*/ | |||
@Deprecated |
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.
Per conversation with @edewata, I think we should un-deprecate these as well.
@@ -37,7 +37,9 @@ public native void verifyKeyIsOnToken(PK11Token token) | |||
* Returns a new CryptoToken where this key resides. | |||
* | |||
* @return The PK11Token that owns this key. | |||
* @deprecated getUniqueID() in PrivateKey has been deprecated |
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.
Per conversation with @edewata, I think we should un-deprecate these as well.
*/ | ||
@Deprecated |
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.
Actually, don't move it, just remove it. :)
@@ -215,6 +215,10 @@ public boolean engineContainsAlias(String alias) { | |||
return getAliases().contains(alias); | |||
} | |||
|
|||
/** | |||
* @deprecated getUniqueID() in PrivateKey has been deprecated |
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.
Per conversation with @edewata, I think we should un-deprecate these as well. So, please remove this annotation.
@@ -402,6 +406,10 @@ public String engineGetCertificateAlias(Certificate cert) { | |||
return null; | |||
} | |||
|
|||
/** | |||
* @deprecated getUniqueID() in PrivateKey has been deprecated |
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 here.
- Addresses https://pagure.io/jss/issue/19 - Modified in response to review requests - Alex Scheel: Updated to fix remaining review comments Signed-off-by: Alexander Scheel <ascheel@redhat.com>
Cool, this looks like a much better list: diff ./jss_master_build.out.warnings ./jss_branch_build.out.warnings
2,10c2,7
< ./org/mozilla/jss/pkcs11/PK11Cipher.java:102: warning: [deprecation] isPadded() in EncryptionAlgorithm has been deprecated
< ./org/mozilla/jss/pkcs11/PK11Cipher.java:105: warning: [deprecation] isPadded() in EncryptionAlgorithm has been deprecated
< ./org/mozilla/jss/pkcs11/PK11Cipher.java:127: warning: [deprecation] isPadded() in EncryptionAlgorithm has been deprecated
< ./org/mozilla/jss/pkcs11/PK11Cipher.java:130: warning: [deprecation] isPadded() in EncryptionAlgorithm has been deprecated
< ./org/mozilla/jss/pkcs11/PK11Cipher.java:165: warning: [deprecation] isPadded() in EncryptionAlgorithm has been deprecated
< ./org/mozilla/jss/pkcs11/PK11Cipher.java:192: warning: [deprecation] isPadded() in EncryptionAlgorithm has been deprecated
< ./org/mozilla/jss/pkcs11/PK11InternalTokenCert.java:16: warning: [deprecation] getUniqueID() in TokenCertificate has been deprecated
< ./org/mozilla/jss/pkcs11/PK11PrivKey.java:43: warning: [deprecation] getUniqueID() in PrivateKey has been deprecated
< ./org/mozilla/jss/pkcs11/PK11TokenCert.java:14: warning: [deprecation] getUniqueID() in TokenCertificate has been deprecated
---
> ./org/mozilla/jss/pkcs11/CipherContextProxy.java:16: warning: [deprecation] finalize() in NativeProxy has been deprecated
> ./org/mozilla/jss/pkcs11/CipherContextProxy.java:17: warning: [deprecation] finalize() in NativeProxy has been deprecated
> ./org/mozilla/jss/pkcs11/KeyProxy.java:12: warning: [deprecation] finalize() in NativeProxy has been deprecated
> ./org/mozilla/jss/pkcs11/KeyProxy.java:13: warning: [deprecation] finalize() in NativeProxy has been deprecated
> ./org/mozilla/jss/pkcs11/ModuleProxy.java:16: warning: [deprecation] finalize() in NativeProxy has been deprecated
> ./org/mozilla/jss/pkcs11/ModuleProxy.java:17: warning: [deprecation] finalize() in NativeProxy has been deprecated
20,22d16
< ./org/mozilla/jss/provider/java/security/JSSKeyStoreSpi.java:189: warning: [deprecation] getUniqueID() in PrivateKey has been deprecated
< ./org/mozilla/jss/provider/java/security/JSSKeyStoreSpi.java:264: warning: [deprecation] getUniqueID() in PrivateKey has been deprecated
< ./org/mozilla/jss/provider/java/security/JSSKeyStoreSpi.java:445: warning: [deprecation] getUniqueID() in PrivateKey has been deprecated
25a20,21
> ./org/mozilla/jss/ssl/SocketProxy.java:15: warning: [deprecation] finalize() in NativeProxy has been deprecated
> ./org/mozilla/jss/ssl/SocketProxy.java:16: warning: [deprecation] finalize() in NativeProxy has been deprecated
34,45c30,39
< ./org/mozilla/jss/tests/Constants.java:108: warning: [deprecation] SSL3_RSA_WITH_NULL_SHA in SSLSocket has been deprecated
< ./org/mozilla/jss/tests/Constants.java:109: warning: [deprecation] SSL3_RSA_WITH_NULL_MD5 in SSLSocket has been deprecated
< ./org/mozilla/jss/tests/Constants.java:85: warning: [deprecation] SSL3_RSA_WITH_RC4_128_MD5 in SSLSocket has been deprecated
< ./org/mozilla/jss/tests/Constants.java:86: warning: [deprecation] SSL3_RSA_WITH_RC4_128_SHA in SSLSocket has been deprecated
< ./org/mozilla/jss/tests/Constants.java:90: warning: [deprecation] SSL3_DHE_RSA_WITH_3DES_EDE_CBC_SHA in SSLSocket has been deprecated
< ./org/mozilla/jss/tests/Constants.java:91: warning: [deprecation] SSL3_DHE_DSS_WITH_3DES_EDE_CBC_SHA in SSLSocket has been deprecated
< ./org/mozilla/jss/tests/Constants.java:95: warning: [deprecation] SSL3_RSA_WITH_3DES_EDE_CBC_SHA in SSLSocket has been deprecated
< ./org/mozilla/jss/tests/Constants.java:96: warning: [deprecation] SSL3_DHE_RSA_WITH_DES_CBC_SHA in SSLSocket has been deprecated
< ./org/mozilla/jss/tests/Constants.java:97: warning: [deprecation] SSL3_DHE_DSS_WITH_DES_CBC_SHA in SSLSocket has been deprecated
< ./org/mozilla/jss/tests/Constants.java:99: warning: [deprecation] SSL3_RSA_WITH_DES_CBC_SHA in SSLSocket has been deprecated
< ./org/mozilla/jss/tests/KeyWrapping.java:86: warning: [deprecation] getUniqueID() in PrivateKey has been deprecated
< ./org/mozilla/jss/tests/KeyWrapping.java:95: warning: [deprecation] getUniqueID() in PrivateKey has been deprecated
---
> ./org/mozilla/jss/tests/Constants.java:104: warning: [deprecation] SSL3_RSA_WITH_RC4_128_MD5 in SSLSocket has been deprecated
> ./org/mozilla/jss/tests/Constants.java:105: warning: [deprecation] SSL3_RSA_WITH_RC4_128_SHA in SSLSocket has been deprecated
> ./org/mozilla/jss/tests/Constants.java:109: warning: [deprecation] SSL3_DHE_RSA_WITH_3DES_EDE_CBC_SHA in SSLSocket has been deprecated
> ./org/mozilla/jss/tests/Constants.java:110: warning: [deprecation] SSL3_DHE_DSS_WITH_3DES_EDE_CBC_SHA in SSLSocket has been deprecated
> ./org/mozilla/jss/tests/Constants.java:114: warning: [deprecation] SSL3_RSA_WITH_3DES_EDE_CBC_SHA in SSLSocket has been deprecated
> ./org/mozilla/jss/tests/Constants.java:115: warning: [deprecation] SSL3_DHE_RSA_WITH_DES_CBC_SHA in SSLSocket has been deprecated
> ./org/mozilla/jss/tests/Constants.java:116: warning: [deprecation] SSL3_DHE_DSS_WITH_DES_CBC_SHA in SSLSocket has been deprecated
> ./org/mozilla/jss/tests/Constants.java:118: warning: [deprecation] SSL3_RSA_WITH_DES_CBC_SHA in SSLSocket has been deprecated
> ./org/mozilla/jss/tests/Constants.java:127: warning: [deprecation] SSL3_RSA_WITH_NULL_SHA in SSLSocket has been deprecated
> ./org/mozilla/jss/tests/Constants.java:128: warning: [deprecation] SSL3_RSA_WITH_NULL_MD5 in SSLSocket has been deprecated We've:
So I think we're good for merging. Thanks @emaldona for all your work and being patient while we worked through |
Addresses https://pagure.io/jss/issue/19
Replaces the previous pull request
emaldona@5305574
that failed tests.