8296143: CertAttrSet's set/get mechanism is not type-safe #10959

wangweij · 2022-11-02T19:52:54Z

The major change is to remove the get and set methods in various CertAttrSet child classes and change them to setXyz and getXyz methods. The Xyz words might come from the field name or the attribute name. For example, X509CertInfo now has setExtensions and setValidity instead of set("extensions", exts) and set("validity", validity). This also has the benefit to remove a lot of try-catch blocks on IOExceptions on "unknown attributes" because everything is known now. At the same time, all the identifier name and attribute names are removed from CertAttrSet child classes. The only left is NAME in extensions since it's still used as keys in CertificateExtensions.

Besides assigning a new value to an internal field, the original set methods might also re-encode by calling encodeThis, invalidate the cached encoding (in X509CertInfo), or check for read-only flag (in X509CertImp). Newly added setXyz methods are doing the same. This is one place that future new setter methods should remember.

Most get implementations simply return an internal field. One exception in X509CertImpl is that when getting something inside the X509CertInfo, it wraps exceptions into a new CertificateParsingException. This is actually related to the way CertificateExtensions::get is implemented where an exception is thrown when an extension does not exist. CertificateExtensions::getExtension has been rewritten to follow the CRLExtensions::getExtension style where null is returned in this case.

The only method left in CertAttrSet is encode, and it no longer throws a CertificateException.

Several classes do have their attributes, and still has get/set methods. This includes CertificateExtensions, CRLExtensions, ReasonFlags, KeyUsageExtension, and NetscapeCertTypeExtensions. Some methods are renamed to be clearer. For example, in CertificateExtensions, we have getExtension instead of get.

There are no more AttributeNameEnumeration.java and X509AttributeName.java.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8296143: CertAttrSet's set/get mechanism is not type-safe

Reviewers

Sean Mullan (@seanjmullan - Reviewer) ⚠️ Review applies to a62ef66d

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/10959/head:pull/10959
$ git checkout pull/10959

Update a local copy of the PR:
$ git checkout pull/10959
$ git pull https://git.openjdk.org/jdk pull/10959/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 10959

View PR using the GUI difftool:
$ git pr show -t 10959

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/10959.diff

bridgekeeper · 2022-11-02T19:54:10Z

👋 Welcome back weijun! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2022-11-02T19:56:58Z

@wangweij The following label will be automatically applied to this pull request:

security

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.

mlbridge · 2022-11-02T20:00:27Z

Webrevs

wangweij · 2022-11-02T21:17:46Z

In fact, setter methods in all extensions can be removed since they are only called inside tests.

src/java.base/share/classes/java/security/cert/X509CertSelector.java

src/java.base/share/classes/sun/security/provider/certpath/DistributionPointFetcher.java

mcpowers · 2022-11-04T02:34:34Z

Way easier to read. Amazing work!

turbanoff · 2022-11-06T14:37:34Z

src/java.base/share/classes/sun/security/x509/CertificateX509Key.java

-                                  "CertAttrSet: CertificateX509Key.");
-        }
+    public PublicKey getKey() {
+        return(key);


Suggested change

return(key);

return key;

Thanks. I was just copying lines from the old get method. Will fix.

seanjmullan · 2022-11-08T17:37:26Z

src/java.base/share/classes/sun/security/x509/CertAttrSet.java

 */
-public interface CertAttrSet<T> {
+public interface CertAttrSet {


Should we rename this to "CertAttr" since I think it now supports a single attribute?

Since it only has encode now, I'll merge it with DerEncoder and remove it in my next step. Therefore I haven't cared about giving it a better name. I had thought about including the merge inside this change but there are a lot of s/derEncode/encode/ renaming and it's better to be done separately.

seanjmullan

Great stuff, just a few minor comments.

seanjmullan · 2022-11-08T17:38:42Z

src/java.base/share/classes/java/security/cert/X509CertSelector.java

@@ -1310,7 +1310,7 @@ public String getIssuerAsString() {
     * @throws IOException if an encoding error occurs
     */
    public byte[] getIssuerAsBytes() throws IOException {
-        return (issuer == null ? null: issuer.getEncoded());
+        return issuer == null ? null: issuer.getEncoded();


Nit, add space between "null" and ":".

seanjmullan · 2022-11-08T17:40:57Z

src/java.base/share/classes/sun/security/pkcs/PKCS9Attribute.java

@@ -687,7 +683,7 @@ public ObjectIdentifier getOID() {
    public String getName() {
        String n = oid.toString();
        KnownOIDs os = KnownOIDs.findMatch(n);
-        return (os == null? n : os.stdName());
+        return os == null? n : os.stdName();


Nit: add space between "null" and "?".

seanjmullan · 2022-11-08T17:41:32Z

src/java.base/share/classes/sun/security/provider/certpath/Builder.java

@@ -204,7 +204,7 @@ static int hops(GeneralNameInterface base, GeneralNameInterface test,
            /* base is ancestor of test */
        case GeneralNameInterface.NAME_NARROWS:
            /* base is descendant of test */
-            return (test.subtreeDepth()-base.subtreeDepth());
+            return test.subtreeDepth()-base.subtreeDepth();


Nit: add spaces around "-".

seanjmullan · 2022-11-08T17:47:49Z

src/java.base/share/classes/sun/security/tools/keytool/Main.java

-        info.set(X509CertInfo.SUBJECT,
-                    dname==null?req.getSubjectName():new X500Name(dname));
+        info.setKey(new CertificateX509Key(req.getSubjectPublicKeyInfo()));
+        info.setSubject(dname==null?req.getSubjectName():new X500Name(dname));


Add spaces between "?" and ":".

seanjmullan · 2022-11-08T17:57:53Z

src/java.base/share/classes/sun/security/x509/CertificatePoliciesExtension.java

-                                "CertAttrSet:CertificatePoliciesExtension.");
-        }
+    public List<PolicyInformation> getCertPolicies() {
+        //XXXX May want to consider cloning this


I would remove this comment. This method is internal and as long as the List is not exposed via a public API (please double-check), a clone is not necessary.

seanjmullan · 2022-11-08T17:59:08Z

src/java.base/share/classes/sun/security/x509/ExtendedKeyUsageExtension.java

-                                "CertAttrSet:ExtendedKeyUsageExtension.");
-        }
+    public Vector<ObjectIdentifier> getUsages() {
+        //XXXX May want to consider cloning this


Remove comment if returned Vector cannot be accessed via public APIs.

openjdk · 2022-11-08T19:46:55Z

@wangweij 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:

8296143: CertAttrSet's set/get mechanism is not type-safe

Reviewed-by: mullan

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 72 new commits pushed to the master branch:

0ee25de: 8296504: Memory leak in G1PLABAllocator::PLABData
dd5d4df: 8295658: G1: Refactor G1SegmentedArray to indicate that it is an allocator
cf65605: 8296445: C++ syntax error in jdwpTransport.h
1169dc0: 8296447: RISC-V: Make the operands order of vrsub_vx/vrsub_vi consistent with RVV 1.0 spec
4c80dff: 8296435: RISC-V: Small refactoring for increment/decrement
47d2c7b: 8295376: Improve debug agent virtual thread performance when no debugger is attached
76790ad: 8295673: Deprecate and disable legacy parallel class loading workaround for non-parallel-capable class loaders
b6738c1: 8295663: Rephrase introduction to testing.md
7e85b41: 8296154: [macos] Change "/Applications" to "Applications" in DMG image
60db5f2: 8294020: improve errors for record declarations
... and 62 more: https://git.openjdk.org/jdk/compare/16a041a67a30ad8f3160e211c629c055d3ff2f80...master

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 master branch, type /integrate in a new comment.

reverted:

wangweij · 2022-11-08T22:34:00Z

/integrate

openjdk · 2022-11-08T22:35:41Z

Going to push as commit 671f84b.
Since your change was applied there have been 74 commits pushed to the master branch:

d04d656: 8296433: Encountered null CLD while loading shared lambda proxy class
74f2b16: 8295303: cleanup debug agent's confusing use of EI_GC_FINISH
0ee25de: 8296504: Memory leak in G1PLABAllocator::PLABData
dd5d4df: 8295658: G1: Refactor G1SegmentedArray to indicate that it is an allocator
cf65605: 8296445: C++ syntax error in jdwpTransport.h
1169dc0: 8296447: RISC-V: Make the operands order of vrsub_vx/vrsub_vi consistent with RVV 1.0 spec
4c80dff: 8296435: RISC-V: Small refactoring for increment/decrement
47d2c7b: 8295376: Improve debug agent virtual thread performance when no debugger is attached
76790ad: 8295673: Deprecate and disable legacy parallel class loading workaround for non-parallel-capable class loaders
b6738c1: 8295663: Rephrase introduction to testing.md
... and 64 more: https://git.openjdk.org/jdk/compare/16a041a67a30ad8f3160e211c629c055d3ff2f80...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-11-08T22:36:00Z

@wangweij Pushed as commit 671f84b.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

the fix

8dc7ba3

openjdk bot added the rfr Pull request is ready for review label Nov 2, 2022

openjdk bot added the security security-dev@openjdk.org label Nov 2, 2022

less throws IOException

de2ac66

mcpowers reviewed Nov 4, 2022

View reviewed changes

src/java.base/share/classes/java/security/cert/X509CertSelector.java Outdated Show resolved Hide resolved

mcpowers reviewed Nov 4, 2022

View reviewed changes

src/java.base/share/classes/sun/security/provider/certpath/DistributionPointFetcher.java Outdated Show resolved Hide resolved

wangweij added 2 commits November 4, 2022 09:35

more cleanup

26a1f5c

more cleanup

f4b3d09

turbanoff reviewed Nov 6, 2022

View reviewed changes

return (abc) to return abc

b4453f6

seanjmullan reviewed Nov 8, 2022

View reviewed changes

more cleanup

a62ef66

seanjmullan approved these changes Nov 8, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Nov 8, 2022

PKCS10Attributes::getElements still used by a test

c6ea900

reverted:

openjdk bot added the integrated Pull request has been integrated label Nov 8, 2022

openjdk bot closed this Nov 8, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Nov 8, 2022

wangweij deleted the 8296143 branch November 8, 2022 22:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8296143: CertAttrSet's set/get mechanism is not type-safe #10959

8296143: CertAttrSet's set/get mechanism is not type-safe #10959

wangweij commented Nov 2, 2022 •

edited by openjdk bot

bridgekeeper bot commented Nov 2, 2022

openjdk bot commented Nov 2, 2022

mlbridge bot commented Nov 2, 2022 •

edited

wangweij commented Nov 2, 2022 •

edited

mcpowers commented Nov 4, 2022

turbanoff Nov 6, 2022

wangweij Nov 6, 2022

seanjmullan Nov 8, 2022

wangweij Nov 8, 2022 •

edited

seanjmullan left a comment

seanjmullan Nov 8, 2022

seanjmullan Nov 8, 2022

seanjmullan Nov 8, 2022

seanjmullan Nov 8, 2022

seanjmullan Nov 8, 2022

seanjmullan Nov 8, 2022

openjdk bot commented Nov 8, 2022

wangweij commented Nov 8, 2022

openjdk bot commented Nov 8, 2022

openjdk bot commented Nov 8, 2022

8296143: CertAttrSet's set/get mechanism is not type-safe #10959

8296143: CertAttrSet's set/get mechanism is not type-safe #10959

Conversation

wangweij commented Nov 2, 2022 • edited by openjdk bot

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Nov 2, 2022

openjdk bot commented Nov 2, 2022

mlbridge bot commented Nov 2, 2022 • edited

Webrevs

wangweij commented Nov 2, 2022 • edited

mcpowers commented Nov 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wangweij Nov 8, 2022 • edited

Choose a reason for hiding this comment

seanjmullan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openjdk bot commented Nov 8, 2022

wangweij commented Nov 8, 2022

openjdk bot commented Nov 8, 2022

openjdk bot commented Nov 8, 2022

wangweij commented Nov 2, 2022 •

edited by openjdk bot

mlbridge bot commented Nov 2, 2022 •

edited

wangweij commented Nov 2, 2022 •

edited

wangweij Nov 8, 2022 •

edited