Skip to content
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

8270946: X509CertImpl.getFingerprint should not return the empty String #4891

Closed
wants to merge 3 commits into from

Conversation

@seanjmullan
Copy link
Member

@seanjmullan seanjmullan commented Jul 23, 2021

Please review this fix to change the internal X509CertImpl.getFingerprint method to not return "" as a fingerprint if there is an error generating that fingerprint. Instead, null is now returned, and "" is no longer cached as a valid fingerprint. Although errors generating fingerprints should be very rare, this is a cleaner way to handle them.

Also, debugging messages have been added when there is an exception. And, as a memory/performance improvement, X509CertImpl.getFingerprint now calls X509CertImpl.getEncodedInternal which avoids cloning the encoded bytes if the Certificate is an instance of X509CertImpl.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8270946: X509CertImpl.getFingerprint should not return the empty String

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/4891/head:pull/4891
$ git checkout pull/4891

Update a local copy of the PR:
$ git checkout pull/4891
$ git pull https://git.openjdk.java.net/jdk pull/4891/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4891

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4891.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jul 23, 2021

👋 Welcome back mullan! 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.

Loading

@openjdk openjdk bot added the rfr label Jul 23, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jul 23, 2021

@seanjmullan 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.

Loading

@openjdk openjdk bot added the security label Jul 23, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Jul 23, 2021

Webrevs

Loading

x -> getFingerprint(x, this));
x -> getFingerprintInternal(x, debug));
}

Copy link
Contributor

@wangweij wangweij Jul 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little confused by these methods. Can you inline getFingerprintInternal and rename getFingerprint on line 1936 to getFingerprintInternal?

Loading

Copy link
Member Author

@seanjmullan seanjmullan Jul 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what you mean by inline. Do you mean use an anonymous inner class instead of a lambda for the Function argument to ConcurrentHashMap.computeIfAbsent? Note that getFingerprintInternal calls this.getEncodedInternal so it cannot be static.

Loading

Copy link
Contributor

@wangweij wangweij Jul 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's "inline" in code refactoring. I was thinking about moving the body of the current getFingerprintInternal into its only caller above which is also an instance method. IMO, the other getFingerPrint on line 1936 is more internal.

Loading

Copy link
Member Author

@seanjmullan seanjmullan Jul 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, check out the latest commit to see if it is clearer.

Loading

@@ -154,18 +157,22 @@
static void checkDistrust(X509Certificate[] chain)
throws ValidatorException {
X509Certificate anchor = chain[chain.length-1];
if (FINGERPRINTS.contains(fingerprint(anchor))) {
String fp = fingerprint(anchor);
if (fp != null && FINGERPRINTS.contains(fp)) {
Copy link
Contributor

@wangweij wangweij Jul 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand the original behavior is also bypassing the check if fingerprint cannot be calculated, but this sounds a little irresponsible. Same as in UntrustedCertificates.

Loading

Copy link
Member Author

@seanjmullan seanjmullan Jul 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I see your point, and I agree it makes more sense in this case to fail, so I will change it. However, if this situation were to occur because of a rogue X509Certificate impl, it is also possible for it to return an encoding that generates a different fingerprint.

Loading

Asserts.assertNull(X509CertImpl.getFingerprint("NoSuchAlg", cert, dbg));

// test cert with bad encoding
X509Certificate fcert = new X509CertificateWithBadEncoding(cert);
Copy link
Contributor

@wangweij wangweij Jul 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact, new X509CertImpl() satisfies your requirement perfectly, which is an unpopulated cert with no encoding. It might be a little weird though. You can continue with your choice.

Loading

Copy link
Member Author

@seanjmullan seanjmullan Jul 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although I didn't include it as part of this change, there are a number of other tests that override a few methods of X509Certificate and the code can be changed to use this pattern. I'll file a follow-on issue to clean that up.

Loading

Treat null fingerprint as untrusted.
@openjdk
Copy link

@openjdk openjdk bot commented Jul 26, 2021

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

8270946: X509CertImpl.getFingerprint should not return the empty String

Reviewed-by: weijun

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

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.

Loading

@openjdk openjdk bot added the ready label Jul 26, 2021
@seanjmullan
Copy link
Member Author

@seanjmullan seanjmullan commented Jul 27, 2021

/integrate

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Jul 27, 2021

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

  • 45d277f: 8270308: Arena::Amalloc may return misaligned address on 32-bit
  • fde1831: 8212961: [TESTBUG] vmTestbase/nsk/stress/jni/ native code cleanup
  • bb508e1: 8269753: Misplaced caret in PatternSyntaxException's detail message
  • c3d8e92: 8190753: (zipfs): Accessing a large entry (> 2^31 bytes) leads to a negative initial size for ByteArrayOutputStream
  • eb6da88: Merge
  • b76a838: 8269150: UnicodeReader not translating \u005c\u005d to \]
  • 7ddabbf: 8271175: runtime/jni/FindClassUtf8/FindClassUtf8.java doesn't have to be run in othervm
  • 3c27f91: 8271222: two runtime/Monitor tests don't check exit code
  • 049b2ad: 8015886: java/awt/Focus/DeiconifiedFrameLoosesFocus/DeiconifiedFrameLoosesFocus.java sometimes failed on ubuntu
  • fcc7d59: 8269342: CICrashAt=1 does not always catch first Java method
  • ... and 92 more: https://git.openjdk.java.net/jdk/compare/38694aa970be73d269cb444ea80ebe7085bd9e90...master

Your commit was automatically rebased without conflicts.

Loading

@openjdk openjdk bot closed this Jul 27, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Jul 27, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jul 27, 2021

@seanjmullan Pushed as commit fc80a6b.

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

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants