Fix missing OID mappings #997

fmarco76 · 2024-03-11T15:38:13Z

Pretty print some additional OID:

OCSP No Check - id-pkix-ocsp-nocheck { 1.3.6.1.5.5.7.48.1.5} Inhibit Any Policy {2.5.29.54}
Extended Key Usage:

serverAuth {1 3 6 1 5 5 7 3 1}
clientAuth {1 3 6 1 5 5 7 3 2}
codeSigning {1 3 6 1 5 5 7 3 3}
emailProtection {1 3 6 1 5 5 7 3 4}

fmarco76 · 2024-03-11T16:51:55Z

Sonar does not like the name convention for OID_OCSPSigning and the new added entries because of the case. They are consistent among them and the one already present are used in pki code. We can update all of them and update pki after the merge. @edewata can we go ahead and change them in the master for both?

edewata

I have some comments, but we can address that in a separate PR. Feel free to merge the PR as is.

We can certainly fix the constant names, but to avoid breaking existing code we should deprecate them first, then drop the old constants in a future release.

edewata · 2024-03-11T18:51:54Z

base/src/main/java/org/mozilla/jss/netscape/security/extensions/ExtendedKeyUsageExtension.java

+        { 1, 3, 6, 1, 5, 5, 7, 3, 4 };
+    public static final ObjectIdentifier OID_EMAIL_PROTECTION = new
+            ObjectIdentifier(OID_EMAIL_PROTECTION_STR);
+
    public static final int OID_CODE_SIGNING_STR[] =
        { 1, 3, 6, 1, 5, 5, 7, 3, 3 };
    public static final ObjectIdentifier OID_CODE_SIGNING = new
            ObjectIdentifier(OID_OCSP_SIGNING_STR);


This is an existing code, but I think it should be OID_CODE_SIGNING_STR instead of OID_OCSP_SIGNING_STR. I wonder why we never see a problem caused by this bug.

@ladycfu ^^

edewata · 2024-03-11T18:53:16Z

base/src/main/java/org/mozilla/jss/netscape/security/extensions/ExtendedKeyUsageExtension.java

+    public static final int OID_EMAIL_PROTECTION_STR[] =
+        { 1, 3, 6, 1, 5, 5, 7, 3, 4 };
+    public static final ObjectIdentifier OID_EMAIL_PROTECTION = new
+            ObjectIdentifier(OID_EMAIL_PROTECTION_STR);


To avoid similar bugs we probably should not define the OID multiple times. I'm thinking we can just define this object like this:

public static final ObjectIdentifier OID_EMAIL_PROTECTION = \ new ObjectIdentifier("1.3.6.1.5.5.7.3.4");

Then if we need the String OID we can just call OID_EMAIL_PROTECTION.toString() or something like that.

We should also deprecate the existing OID_OCSPSigning and OID_CODESigning.

edewata · 2024-03-11T18:56:49Z

base/src/main/java/org/mozilla/jss/netscape/security/x509/OIDMap.java

@@ -94,9 +96,12 @@ public class OIDMap {

    public static final String EXT_KEY_USAGE_NAME = "ExtendedKeyUsageExtension";
    public static final String EXT_INHIBIT_ANY_POLICY_NAME = "InhibitAnyPolicyExtension";
+    private static final String EXT_INHIBIT_ANY_POLICY = ROOT + "." + InhibitAnyPolicyExtension.NAME;


Just a note that the InhibitAnyPolicyExtension.NAME is actually pointing back to EXT_INHIBIT_ANY_POLICY_NAME. We probably should move EXT_INHIBIT_ANY_POLICY_NAME into InhibitAnyPolicyExtension.NAME later.

edewata · 2024-03-11T19:03:08Z

base/src/main/java/org/mozilla/jss/netscape/security/x509/OIDMap.java

@@ -132,6 +137,8 @@ private static void loadNamesDefault(Properties props) {
        props.put(AUTH_KEY_IDENTIFIER, "2.5.29.35");
        props.put(SUBJ_DIR_ATTR, "2.5.29.9");
        props.put(EXT_KEY_USAGE, "2.5.29.37");
+        props.put(EXT_INHIBIT_ANY_POLICY, "2.5.29.54");


This OID is already defined in InhibitAnyPolicyExtension.OID. Should we use that instead?

edewata · 2024-03-11T19:03:45Z

base/src/main/java/org/mozilla/jss/netscape/security/x509/OIDMap.java

@@ -132,6 +137,8 @@ private static void loadNamesDefault(Properties props) {
        props.put(AUTH_KEY_IDENTIFIER, "2.5.29.35");
        props.put(SUBJ_DIR_ATTR, "2.5.29.9");
        props.put(EXT_KEY_USAGE, "2.5.29.37");
+        props.put(EXT_INHIBIT_ANY_POLICY, "2.5.29.54");
+        props.put(OCSP_NO_CHECK, "1.3.6.1.5.5.7.48.1.5");


This is also already defined in OCSPNoCheckExtension.OID. Should we use that one?

edewata · 2024-03-11T19:08:18Z

base/src/main/java/org/mozilla/jss/netscape/security/x509/OIDMap.java

+        props.put(EXT_INHIBIT_ANY_POLICY,
+                "org.mozilla.jss.netscape.security.extensions.InhibitAnyPolicyExtension");
+        props.put(OCSP_NO_CHECK,
+                "org.mozilla.jss.netscape.security.extensions.OCSPNoCheckExtension");


To ensure that the class names and packages are correct we probably should use InhibitAnyPolicyExtension.class.getName() and OCSPNoCheckExtension.class.getName().

fmarco76 · 2024-03-12T09:45:22Z

@edewata I have just read in RHCS-4883 about other OIDs which are not printed. I'll incorporate these other OID and reorganise this code for better maintainability. Moving this PR to draft for the moment. Thanks

Pretty print some additional OID: OCSP No Check - id-pkix-ocsp-nocheck { 1.3.6.1.5.5.7.48.1.5} Inhibit Any Policy {2.5.29.54} Extended Key Usage: - serverAuth {1 3 6 1 5 5 7 3 1} - clientAuth {1 3 6 1 5 5 7 3 2} - codeSigning {1 3 6 1 5 5 7 3 3} - emailProtection {1 3 6 1 5 5 7 3 4} - id-kp-ipsecIKE {1 3 6 1 5 5 7 3 17} - iKEIntermediate {1 3 6 1 5 5 8 2 2}

fmarco76 · 2024-03-13T18:21:04Z

@edewata I have reorganised the code and going deeply I found the real reason some of the extension were not shown. There was a static code which was never invoked when used with PrettyPrintCert because the extension class was never loaded. The reference to the class were resolved at compile time since they are static final. I have moved the loading code to a different place and it works with PrettyPrintCert and it should work with PKI since OIDMap is always loaded to access the maps.

There is a problem with postgresql but it is also in other PR. We can investigate in a separate PR.

edewata

Thanks for the update! I have some more comments but the changes look good. Feel free to update/merge.

edewata · 2024-03-13T22:57:53Z

base/src/main/java/org/mozilla/jss/netscape/security/util/ExtPrettyPrint.java

+                if (oid.equals(ExtendedKeyUsageExtension.OID_IKE_INTERMEDIATE)) {
+                    sb.append(pp.indent(mIndentSize + 8) + "ipsec Intermediate System Usage" +
+                            " - " + oid.toString() + "\n");
+                } else if (oid.equals(ExtendedKeyUsageExtension.OID_ID_KP_IPSEC_IKE)) {
+                    sb.append(pp.indent(mIndentSize + 8) + "ipsec Internet Key Exchange" +
+                            " - " + oid.toString() + "\n");
+                } else if (oid.equals(ExtendedKeyUsageExtension.OID_OCSP_SIGNING)) {
+                    sb.append(pp.indent(mIndentSize + 8) + "OCSPSigning" +
+                            " - " + oid.toString() + "\n");
+                } else if (oid.equals(ExtendedKeyUsageExtension.OID_EMAIL_PROTECTION)) {
+                    sb.append(pp.indent(mIndentSize + 8) + "emailProtection" +
+                            " - " + oid.toString() + "\n");
+                } else if (oid.equals(ExtendedKeyUsageExtension.OID_CODE_SIGNING)) {
+                    sb.append(pp.indent(mIndentSize + 8) + "codeSigning" +
+                            " - " + oid.toString() + "\n");
+                } else if (oid.equals(ExtendedKeyUsageExtension.OID_CLIENT_AUTH)) {
+                    sb.append(pp.indent(mIndentSize + 8) + "clientAuth" +
+                            " - " + oid.toString() + "\n");
+                } else if (oid.equals(ExtendedKeyUsageExtension.OID_SERVER_AUTH)) {
+                    sb.append(pp.indent(mIndentSize + 8) + "serverAuth" +
+                            " - " + oid.toString() + "\n");
                } else {
                    sb.append(pp.indent(mIndentSize + 8) + oid.toString() + "\n");
                }


The toString() is redundant since it's implicitly called during string concatenation.

edewata · 2024-03-13T23:13:01Z

base/src/main/java/org/mozilla/jss/netscape/security/x509/OIDMap.java

@@ -131,43 +143,28 @@ private static void loadNamesDefault(Properties props) {
        props.put(CERT_POLICIES, "2.5.29.32");
        props.put(AUTH_KEY_IDENTIFIER, "2.5.29.35");
        props.put(SUBJ_DIR_ATTR, "2.5.29.9");
-        props.put(EXT_KEY_USAGE, "2.5.29.37");


Just to confirm, so the EXT_KEY_USAGE doesn't need to be specified here anymore?

The ExtendedKeyUsageExtension class was adding itself here. I have not investigated there reason but it is like adding this extension whatever is the map. However, this code was not executed so I moved in the static block of the OIDMap with the addClass() method.

edewata · 2024-03-13T23:13:51Z

base/src/main/java/org/mozilla/jss/netscape/security/x509/OIDMap.java

-        props.put(EXT_KEY_USAGE,
-                  "org.mozilla.jss.netscape.security.extensions.ExtendedKeyUsageExtension");


Same thing, so the EXT_KEY_USAGE doesn't need to be specified here anymore?

Same anwer.

Static code inside the extension is not loaded by PrettyPrintCert because only static final values of the class are referenced and these values are resolved at compile time. The static initialisation has moved from static block in the extension to the OIDMap static block which is the class loaded at runtime. Useless OID string from ExtendedKeyUsageExtension has been removed from previous commit and existing OIDs (ocsp signing and code signing) have been deprecated.

sonarqubecloud · 2024-03-14T09:23:15Z

Quality Gate passed

Issues
10 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

fmarco76 · 2024-03-14T09:48:20Z

@edewata Thanks! I have just remove the useless toString() before merge.

fmarco76 requested a review from edewata March 11, 2024 15:38

edewata approved these changes Mar 11, 2024

View reviewed changes

fmarco76 marked this pull request as draft March 12, 2024 09:45

fmarco76 force-pushed the FixMissOIDPrint branch 4 times, most recently from 416fe84 to 4854289 Compare March 13, 2024 18:03

fmarco76 requested a review from edewata March 13, 2024 18:13

fmarco76 marked this pull request as ready for review March 13, 2024 18:35

edewata approved these changes Mar 13, 2024

View reviewed changes

fmarco76 force-pushed the FixMissOIDPrint branch from 4854289 to c241928 Compare March 14, 2024 09:20

fmarco76 merged commit 5ecb67d into dogtagpki:master Mar 14, 2024
34 of 35 checks passed

fmarco76 deleted the FixMissOIDPrint branch March 14, 2024 10:05

fmarco76 mentioned this pull request May 17, 2024

Fix EKU decoding #1007

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix missing OID mappings #997

Fix missing OID mappings #997

fmarco76 commented Mar 11, 2024

fmarco76 commented Mar 11, 2024

edewata left a comment

edewata Mar 11, 2024

edewata Mar 11, 2024

edewata Mar 11, 2024

edewata Mar 11, 2024

edewata Mar 11, 2024

edewata Mar 11, 2024

fmarco76 commented Mar 12, 2024

fmarco76 commented Mar 13, 2024

edewata left a comment

edewata Mar 13, 2024

edewata Mar 13, 2024

fmarco76 Mar 14, 2024

edewata Mar 13, 2024

fmarco76 Mar 14, 2024

sonarqubecloud bot commented Mar 14, 2024

fmarco76 commented Mar 14, 2024

		props.put(EXT_KEY_USAGE,
		"org.mozilla.jss.netscape.security.extensions.ExtendedKeyUsageExtension");

Fix missing OID mappings #997

Fix missing OID mappings #997

Conversation

fmarco76 commented Mar 11, 2024

fmarco76 commented Mar 11, 2024

edewata 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

fmarco76 commented Mar 12, 2024

fmarco76 commented Mar 13, 2024

edewata 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

sonarqubecloud bot commented Mar 14, 2024

Quality Gate passed

fmarco76 commented Mar 14, 2024