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

8281561: Disable http DIGEST mechanism with MD5 and SHA-1 by default #7688

Closed
wants to merge 25 commits into from

Conversation

Michael-Mc-Mahon
Copy link
Member

@Michael-Mc-Mahon Michael-Mc-Mahon commented Mar 4, 2022

Hi,

Could I get the following change reviewed please, which is to disable the MD5 message digest algorithm by default in the HTTP Digest authentication mechanism? The algorithm can be opted into by setting a new system property "http.auth.digest.reEnabledAlgs" to include the value MD5. The change also updates the Digest authentication implementation to use some of the more secure features defined in RFC7616, such as username hashing and additional digest algorithms like SHA256 and SHA512-256.

  • Michael

Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed
  • Change requires a CSR request to be approved

Issues

  • JDK-8281561: Disable http DIGEST mechanism with MD5 and SHA-1 by default
  • JDK-8282649: Disable http DIGEST mechanism with MD5 by default (CSR)

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 7688

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

Using diff file

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

@Michael-Mc-Mahon
Copy link
Member Author

/csr needed

@bridgekeeper
Copy link

bridgekeeper bot commented Mar 4, 2022

👋 Welcome back michaelm! 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 openjdk bot added the csr Pull request needs approved CSR before integration label Mar 4, 2022
@openjdk
Copy link

openjdk bot commented Mar 4, 2022

@Michael-Mc-Mahon has indicated that a compatibility and specification (CSR) request is needed for this pull request.
@Michael-Mc-Mahon this pull request must refer to an issue in JBS to be able to link it to a CSR request. To refer this pull request to an issue in JBS, please use the /issue command in a comment in this pull request.

@openjdk
Copy link

openjdk bot commented Mar 4, 2022

@Michael-Mc-Mahon The following label will be automatically applied to this pull request:

  • net

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.

@openjdk openjdk bot added the net net-dev@openjdk.org label Mar 4, 2022
@Michael-Mc-Mahon
Copy link
Member Author

I have still to add the doc update describing the system property, which will come shortly. I may suggest a change of name to the system property to better reflect its exact meaning/purpose

@openjdk openjdk bot added the rfr Pull request is ready for review label Mar 4, 2022
@mlbridge
Copy link

mlbridge bot commented Mar 4, 2022

@dfuch
Copy link
Member

dfuch commented Mar 4, 2022

Should we instead have a property to disable algorithms, whose default value would contain "MD5" by default?

@Michael-Mc-Mahon
Copy link
Member Author

Should we instead have a property to disable algorithms, whose default value would contain "MD5" by default?

I considered that and implemented it that way at the start, but what you would end up with then is users running their code with something like: -DdisabledAlgNames=""

I find that style leads to a much less explicit "opting in" than by making the user explicitly identify the deprecated algorithm by name.

@dfuch
Copy link
Member

dfuch commented Mar 4, 2022

I considered that and implemented it that way at the start, but what you would end up with then is users running their code with something like: -DdisabledAlgNames=""

I find that style leads to a much less explicit "opting in" than by making the user explicitly identify the deprecated algorithm by name.

Right - but it would also allow users to opt-in to disable more algorithms by listing them in the property

@Michael-Mc-Mahon
Copy link
Member Author

Michael-Mc-Mahon commented Mar 4, 2022

I considered that and implemented it that way at the start, but what you would end up with then is users running their code with something like: -DdisabledAlgNames=""
I find that style leads to a much less explicit "opting in" than by making the user explicitly identify the deprecated algorithm by name.

Right - but it would also allow users to opt-in to disable more algorithms by listing them in the property

In practical terms, the only other likely candidate there is SHA-1. If that weren't the case, I'd disagree with your point.

So, maybe, we could have a 2nd net property with the default disabled algorithms and in net.properties we identify MD5 only for now. Users could add to that list if they want or even specify it on the command line. I think it's potentially confusing, but maybe there is a case for adding to the disabled list. I need to think about a way to do this without subvertng the point about making the user explicitly opt in.

@Michael-Mc-Mahon
Copy link
Member Author

I considered that and implemented it that way at the start, but what you would end up with then is users running their code with something like: -DdisabledAlgNames=""
I find that style leads to a much less explicit "opting in" than by making the user explicitly identify the deprecated algorithm by name.

Right - but it would also allow users to opt-in to disable more algorithms by listing them in the property

In practical terms, the only other likely candidate there is SHA-1. If that weren't the case, I'd disagree with your point.

So, maybe, we could have a 2nd net property with the default disabled algorithms and in net.properties we identify MD5 only for now. Users could add to that list if they want or even specify it on the command line. I think it's potentially confusing, but maybe there is a case for adding to the disabled list. I need to think about a way to do this without subvertng the point about making the user explicitly opt in.

I considered that and implemented it that way at the start, but what you would end up with then is users running their code with something like: -DdisabledAlgNames=""
I find that style leads to a much less explicit "opting in" than by making the user explicitly identify the deprecated algorithm by name.

Right - but it would also allow users to opt-in to disable more algorithms by listing them in the property

In practical terms, the only other likely candidate there is SHA-1. If that weren't the case, I'd disagree with your point.

So, maybe, we could have a 2nd net property with the default disabled algorithms and in net.properties we identify MD5 only for now. Users could add to that list if they want or even specify it on the command line. I think it's potentially confusing, but maybe there is a case for adding to the disabled list. I need to think about a way to do this without subvertng the point about making the user explicitly opt in.

Thinking about it again, I wonder if we should just deprecate SHA-1 at the same time. I think there will be less compatibility impact than with MD5, and it's basically broken as well. I don't see a reason to opt out of other algorithms at this time.

@dfuch
Copy link
Member

dfuch commented Mar 4, 2022

So, maybe, we could have a 2nd net property with the default disabled algorithms and in net.properties we identify MD5 only for now. Users could add to that list if they want or even specify it on the command line. I think it's potentially confusing, but maybe there is a case for adding to the disabled list. I need to think about a way to do this without subvertng the point about making the user explicitly opt in.

Thinking about it again, I wonder if we should just deprecate SHA-1 at the same time. I think there will be less compatibility impact than with MD5, and it's basically broken as well. I don't see a reason to opt out of other algorithms at this time.

I see - maybe we should have a security property identifying the list of algorithm that are disabled, and then a system property to reenable them?

Comment on lines 70 to 71
private static final Set<String> defDisabledAlgs =
Set.of("MD5");
Copy link
Member

Choose a reason for hiding this comment

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

What I'm suggesting is that the content of this set could be seeded with the value of a Security Property, defined in java.security - rather than have the default value hardcoded here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I think that should work

@@ -224,6 +224,12 @@ <H2>Misc HTTP URL stream protocol handler properties</H2>
property is defined, then its value will be used as the domain
name.</P>
</OL>
<LI><P><B>{@systemProperty http.auth.digest.reEnabledAlgs}</B> (default: &lt;none&gt;)<BR>
Copy link
Member

@jaikiran jaikiran Mar 4, 2022

Choose a reason for hiding this comment

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

Hello Michael, from what I understand of doc-files directory (which is where this html file resides) the javadoc tool considers it an unprocessed files location[1]. So would adding the systemProperty javadoc taglet here be necessary? I think this won't end up being listed in the system properties index generated by the javadoc tool and I think this line here will just get rendered literally.

[1] https://docs.oracle.com/javase/8/docs/technotes/tools/windows/javadoc.html

Copy link
Member

Choose a reason for hiding this comment

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

This actually seems to work. If you build the docs for JDK mainline, you can search for instance, for http.keepAlive.time.proxy in the javadoc search box and it will lead you net-properties.html.

Copy link
Member

Choose a reason for hiding this comment

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

You are right indeed. I just checked the generated system properties index and it does list these properties and even links back to this document. In fact, I even found a mail which clearly states that this processing of @systemProperties is supported for doc-files https://mail.openjdk.java.net/pipermail/core-libs-dev/2018-November/056653.html

if (logger.isLoggable(PlatformLogger.Level.INFO)) {
logger.info(msg + " This constraint may be relaxed by setting " +
"the \"http.auth.digest.enabledDigestAlgs\" system property.");
}
Copy link
Member

Choose a reason for hiding this comment

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

I suspect this is a typo and perhaps should have been http.auth.digest.reEnabledAlgs?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. I'm going to change the name once more and will update it then.

@mlbridge
Copy link

mlbridge bot commented Mar 4, 2022

Mailing list message from Bernd Eckenfels on net-dev:

Hello,

While I like the idea of the user having to explicitely specify the rexenabled legacy algorithms (as opposed to removing the defaultsdisabled) it is not the style the other algorithm policies in JCE work - so it might be confusing.

But, more critically I would separate the enabling/implementing of new algorithms from disabling old ones. Especially since there needs to be changes on the server side first. (And I wonder if this can be negotiated anyway?).

So why not start with a ?provide new DIGEST Mechanisms? change? Having said that, would it need to start out with disabled new mechanisms so the update won?t change the behavior? (If there is no negotiation?)

Gruss
Bernd
--
http://bernd.eckenfels.net
________________________________
Von: net-dev <net-dev-retn at openjdk.java.net> im Auftrag von Michael McMahon <michaelm at openjdk.java.net>
Gesendet: Friday, March 4, 2022 1:33:06 PM
An: net-dev at openjdk.java.net <net-dev at openjdk.java.net>
Betreff: Re: RFR: 8281561: Disable http DIGEST mechanism with MD5 by default

On Fri, 4 Mar 2022 09:37:21 GMT, Michael McMahon <michaelm at openjdk.org> wrote:

Hi,

Could I get the following change reviewed please, which is to disable the MD5 message digest algorithm by default in the HTTP Digest authentication mechanism? The algorithm can be opted into by setting a new system property "http.auth.digest.reEnabledAlgs" to include the value MD5. The change also updates the Digest authentication implementation to use some of the more secure features defined in RFC7616, such as username hashing and additional digest algorithms like SHA256 and SHA512-256.

- Michael

I considered that and implemented it that way at the start, but what you would end up with then is users running their code with something like: -DdisabledAlgNames=""
I find that style leads to a much less explicit "opting in" than by making the user explicitly identify the deprecated algorithm by name.

Right - but it would also allow users to opt-in to disable more algorithms by listing them in the property

In practical terms, the only other likely candidate there is SHA-1. If that weren't the case, I'd disagree with your point.

So, maybe, we could have a 2nd net property with the default disabled algorithms and in net.properties we identify MD5 only for now. Users could add to that list if they want or even specify it on the command line. I think it's potentially confusing, but maybe there is a case for adding to the disabled list. I need to think about a way to do this without subvertng the point about making the user explicitly opt in.

I considered that and implemented it that way at the start, but what you would end up with then is users running their code with something like: -DdisabledAlgNames=""
I find that style leads to a much less explicit "opting in" than by making the user explicitly identify the deprecated algorithm by name.

Right - but it would also allow users to opt-in to disable more algorithms by listing them in the property

In practical terms, the only other likely candidate there is SHA-1. If that weren't the case, I'd disagree with your point.

So, maybe, we could have a 2nd net property with the default disabled algorithms and in net.properties we identify MD5 only for now. Users could add to that list if they want or even specify it on the command line. I think it's potentially confusing, but maybe there is a case for adding to the disabled list. I need to think about a way to do this without subvertng the point about making the user explicitly opt in.

Thinking about it again, I wonder if we should just deprecate SHA-1 at the same time. I think there will be less compatibility impact than with MD5, and it's basically broken as well. I don't see a reason to opt out of other algorithms at this time.

-------------

PR: https://git.openjdk.java.net/jdk/pull/7688
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/net-dev/attachments/20220304/0dd0e778/attachment.htm>

Copy link
Contributor

@wangweij wangweij left a comment

Choose a reason for hiding this comment

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

LGTM now. It will be even nicer if the known answer tests in RFC 7616 can be included.

@seanjmullan
Copy link
Member

Suggest changing the bug title to reflect that SHA-1 is now also disabled by default, i.e. "Disable http DIGEST mechanisms with MD5 or SHA-1 by default".

@openjdk openjdk bot removed the csr Pull request needs approved CSR before integration label Mar 23, 2022
@openjdk
Copy link

openjdk bot commented Mar 23, 2022

@Michael-Mc-Mahon 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:

8281561: Disable http DIGEST mechanism with MD5 and SHA-1 by default

Reviewed-by: weijun, dfuchs

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

  • 0c472c8: 8283756: (zipfs) ZipFSOutputStreamTest.testOutputStream should only check inflated bytes
  • d6fa8b0: 8283469: Don't use memset to initialize members in FileMapInfo and fix memory leak
  • 8567266: 8283683: Make ThreadLocalRandom a final class
  • f4eaa16: 8283728: jdk.hotspot.agent: Wrong location for RISCV64ThreadContext.java
  • cdef087: 8283727: P11KeyGenerator has import statement with two semicolons after JDK-8267319

Please see this link for an up-to-date comparison between the source branch of this pull request and 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.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Mar 23, 2022
@Michael-Mc-Mahon Michael-Mc-Mahon changed the title 8281561: Disable http DIGEST mechanism with MD5 by default 8281561: Disable http DIGEST mechanism with MD5 and SHA-1 by default Mar 25, 2022
@Michael-Mc-Mahon
Copy link
Member Author

Michael-Mc-Mahon commented Mar 25, 2022

webrevs 06 & 07 have the latest changes, if anyone is just looking at the incremental changes.

The main changes are new tests including the hardcoded test from RFC7616 (which has to be run using a modified JDK).
Also, some small reorganisation of the DigestAuthentication impl code.

if (session) {
algorithm = algorithm.substring(0, algorithm.length() - 5);
}
boolean session = algorithm.endsWith ("-sess");
Copy link
Member

Choose a reason for hiding this comment

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

should that be digest.endsWith("-sess"); ?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, the digest field refers to the actual message digest algorithm (as known to the security libraries). The algorithm field holds the algorithm name as it is defined in RFC7616.

Copy link
Member

Choose a reason for hiding this comment

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

I am confused here - because you converted algorithm to upper case, so it should never end with -sess?

Copy link
Member Author

Choose a reason for hiding this comment

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

Look at line 478: The algorithm field is reset here to be the upper case of the digest name plus the -sess suffix in lower case.

Copy link
Member

Choose a reason for hiding this comment

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

Doh! Somehow I'd read that as digestName = digestName + "-sess".
All clear now.

if (session) {
algorithm = algorithm.substring(0, algorithm.length() - 5);
}
boolean session = algorithm.endsWith ("-sess");
Copy link
Member

Choose a reason for hiding this comment

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

Doh! Somehow I'd read that as digestName = digestName + "-sess".
All clear now.

@Michael-Mc-Mahon
Copy link
Member Author

/integrate

@openjdk
Copy link

openjdk bot commented Mar 28, 2022

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

  • 0c472c8: 8283756: (zipfs) ZipFSOutputStreamTest.testOutputStream should only check inflated bytes
  • d6fa8b0: 8283469: Don't use memset to initialize members in FileMapInfo and fix memory leak
  • 8567266: 8283683: Make ThreadLocalRandom a final class
  • f4eaa16: 8283728: jdk.hotspot.agent: Wrong location for RISCV64ThreadContext.java
  • cdef087: 8283727: P11KeyGenerator has import statement with two semicolons after JDK-8267319

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Mar 28, 2022
@openjdk openjdk bot closed this Mar 28, 2022
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Mar 28, 2022
@openjdk
Copy link

openjdk bot commented Mar 28, 2022

@Michael-Mc-Mahon Pushed as commit 7f2a3ca.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated Pull request has been integrated net net-dev@openjdk.org
5 participants