HTTP Public Key Pinning #3707

TYsewyn · 2016-02-18T22:20:52Z

HTTP Public Key Pinning (HPKP) is a security mechanism which allows HTTPS websites
to resist impersonation by attackers using mis-issued or otherwise fraudulent certificates.
(For example, sometimes attackers can compromise certificate authorities,
and then can mis-issue certificates for a web origin.)
The HTTPS web server serves a list of public key hashes, and on subsequent connections
clients expect that server to use 1 or more of those public keys in its certificate chain.

This commit will add this new functionality.

Fixes gh-3706

TYsewyn · 2016-02-18T22:21:04Z

I have signed and agree to the terms of the SpringSource Individual Contributor License Agreement.

rwinch · 2016-02-19T02:36:43Z

web/src/main/java/org/springframework/security/web/header/writers/HpkpHeaderWriter.java

+ * @author Tim Ysewyn
+ * @since 4.1
+ */
+public final class HpkpHeaderWriter implements HeaderWriter {


I think it would be valuable to provide example values for every javadoc argument.

I'm not quite sure what you want here. I already added some examples on top of the class.
Do you mean we should provide an example outcome after executing each method?

@TYsewyn For example, you might include the following for setMaxAgeInSeconds

@param maxAgeInSeconds the maximum amount of time (in seconds) to regard the host as a known pinned host. (i.e. TimeUnit.DAYS.toSeconds(30) would set this to 30 days)

You might provide information on setPins that explains the value of the map.

... original javadoc ... To get a pin of Public-Key-Pins: pin-sha256="d6qzRu9zOECb90Uez27xWltNsj0e1Md7GkYYkVoZWmM="; pin-sha256="LPJNul+wow4m6DsqxbninhsWHlwfp0JecwQzYpOLmCQ=" Map<String,String> pins = new HashMap<String,String>(); pins.put("LPJNul+wow4m6DsqxbninhsWHlwfp0JecwQzYpOLmCQ=", "sha256"); pins.put("E9CZ9INDbd+2eRQozYqqbQ2yXLVKB9+xcprMF+44U1g=", "sha256");

Got it! By the end of the day (GMT+1 over here) you'll have the fixes!

@TYsewyn Since only sha256 pin is currently supported perhaps we should have a method that allows adding a (or a set of) sha256 pin(s)?

We might also provide details on how to calculate the hash (or a link to something rather permanent that describes how).

rwinch · 2016-02-19T02:38:18Z

Thanks for the PR @TYsewyn :) Overall this looks very good! I have provided one comment inline. One additional item is listed below.

If necessary we can likely divide some of this work into distinct tasks, but ultimately I think the feature will get a lot more use if we provide a few additional items (in order of preference):

Updates to the documentation
Java Configuration Support
XML Configuration Support

What are your thoughts? Would you consider updating the PR with the additional items?

TYsewyn · 2016-02-19T06:14:22Z

I'll start working on it later today.
Hopefully I will get it done by the end of the weekend :)

Is the Java and XML configuration support located in the config module? Couldn't find it yesterday, but I'll probably have overlooked it..

TYsewyn · 2016-02-20T00:10:00Z

I'm currently working on the XML configuration support and created a version for 4.1.
Is it ok if I bump the default XSD version from 4.0 to 4.1?

rwinch · 2016-02-20T02:44:12Z

@TYsewyn Awesome to hear! Yes please bump it up.

TYsewyn · 2016-02-20T02:46:02Z

Making the final changes as we speak.. Only need to run some tests en squash some commits :)

TYsewyn · 2016-02-20T03:04:11Z

I just noticed that I didn't wrote any test for the java configuration, woops.. Will fix that tomorrow!

rwinch · 2016-02-20T03:10:28Z

@TYsewyn Awesome news! I'm looking forward to the update with the tests!

TYsewyn · 2016-02-20T03:12:04Z

In the meantime, have some fun with the new XML config support ;)

TYsewyn · 2016-02-20T16:43:03Z

From what I can tell, I think this feature is complete and ready to be shipped with 4.1 @rwinch! :)

TYsewyn · 2016-02-25T22:24:02Z

@rwinch Did you already have the time to review this PR by any chance? :)

rwinch · 2016-02-25T22:36:34Z

@TYsewyn Sorry I haven't had time just yet. I've been busy getting Spring Session 1.1 released. Now I am working on Spring Security 4.0.4.RELEASE (out tomorrow). Then finally I will be getting to some PRs for Spring Security, Spring Session, and Spring LDAP. This means you should expect to hear from me by Monday.

rwinch · 2016-03-01T04:54:40Z

...n/java/org/springframework/security/config/annotation/web/configurers/HeadersConfigurer.java

+		 *
+		 * @param reportUri the URI where the browser should send the report to.
+		 */
+		public HpkpConfig reportUri(URI reportUri) {


Might be nice to support a String variant as well (this makes for a easier to read DSL)

Should we also overload .setReportUri(URI reportUri)in HPKPHeaderWriter then?

Good point. Yes

rwinch · 2016-03-01T05:02:39Z

@TYsewyn Thanks again for this PR!

I did a review of the code and the only things that need addressed are:

HTTP Public Key Pinning #3707 (comment) (from previous review)
HTTP Public Key Pinning #3707 (comment)

It is real late here, so I will likely want to take a look at this again first thing in the morning when I have a clear head. If you can get those changes in, then I think we can merge tomorrow :)

Sorry again for the delays on the review and thanks for such an outstanding PR!

rwinch · 2016-03-01T14:13:55Z

web/src/main/java/org/springframework/security/web/header/writers/HpkpHeaderWriter.java

+	 * @param algorithm the cryptographic hash algorithm.
+	 * @throws IllegalArgumentException if algorithm or pin is null
+	 */
+	public void addPin(String pin, String algorithm) {


I would go as far as to say we should change this method to addSha256Pins(String... pins) since the only currently supported pin is sha256 and the setPins method would provide more flexibility if they need additional pin types and we haven't had a chance to update.

TYsewyn · 2016-03-01T21:09:22Z

@rwinch The changes are made as proposed. I also rebased the commits of this feature branch onto the master for easy merging ;-)

rwinch · 2016-03-02T22:54:48Z

docs/manual/src/docs/asciidoc/index.adoc

+	.headers()
+		.httpPublicKeyPinning()
+			.includeSubdomains(true)
+			.reportUri(new URI("http://example.net/pkp-report"))


We should perhaps update to use the new String method

rwinch · 2016-03-02T22:58:45Z

If you are able, squashing your commits is appreciated too. If not, this is something I can do for you.

HTTP Public Key Pinning (HPKP) is a security mechanism which allows HTTPS websites to resist impersonation by attackers using mis-issued or otherwise fraudulent certificates. (For example, sometimes attackers can compromise certificate authorities, and then can mis-issue certificates for a web origin.) The HTTPS web server serves a list of public key hashes, and on subsequent connections clients expect that server to use 1 or more of those public keys in its certificate chain. This commit will add this new functionality. Fixes gh-3706

TYsewyn · 2016-03-04T17:31:54Z

@rwinch Can we close this PR since it is merged?

rwinch · 2016-03-04T17:37:03Z

@TYsewyn Yes thank you!

rwinch reviewed Feb 19, 2016
View reviewed changes

rwinch reviewed Mar 1, 2016
View reviewed changes

rwinch added the status: waiting-for-feedback We need additional information before we can continue label Mar 1, 2016

rwinch reviewed Mar 2, 2016
View reviewed changes

rwinch added status: duplicate A duplicate of another issue in: web An issue in web modules (web, webmvc) New Feature and removed status: waiting-for-feedback We need additional information before we can continue labels Mar 3, 2016

rwinch added this to the 4.1.0 RC1 milestone Mar 3, 2016

rwinch self-assigned this Mar 3, 2016

rwinch closed this Mar 4, 2016

TYsewyn deleted the feature/gh-3706-HPKP branch March 4, 2016 17:55

rwinch added the type: enhancement A general enhancement label May 3, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HTTP Public Key Pinning #3707

HTTP Public Key Pinning #3707

TYsewyn commented Feb 18, 2016

TYsewyn commented Feb 18, 2016

rwinch Feb 19, 2016

TYsewyn Mar 1, 2016

rwinch Mar 1, 2016

TYsewyn Mar 1, 2016

rwinch Mar 1, 2016

rwinch commented Feb 19, 2016

TYsewyn commented Feb 19, 2016

TYsewyn commented Feb 20, 2016

rwinch commented Feb 20, 2016

TYsewyn commented Feb 20, 2016

TYsewyn commented Feb 20, 2016

rwinch commented Feb 20, 2016

TYsewyn commented Feb 20, 2016

TYsewyn commented Feb 20, 2016

TYsewyn commented Feb 25, 2016

rwinch commented Feb 25, 2016

rwinch Mar 1, 2016

TYsewyn Mar 1, 2016

rwinch Mar 1, 2016

rwinch commented Mar 1, 2016

rwinch Mar 1, 2016

TYsewyn commented Mar 1, 2016

rwinch Mar 2, 2016

rwinch commented Mar 2, 2016

TYsewyn commented Mar 4, 2016

rwinch commented Mar 4, 2016

HTTP Public Key Pinning #3707

HTTP Public Key Pinning #3707

Conversation

TYsewyn commented Feb 18, 2016

TYsewyn commented Feb 18, 2016

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

rwinch commented Feb 19, 2016

TYsewyn commented Feb 19, 2016

TYsewyn commented Feb 20, 2016

rwinch commented Feb 20, 2016

TYsewyn commented Feb 20, 2016

TYsewyn commented Feb 20, 2016

rwinch commented Feb 20, 2016

TYsewyn commented Feb 20, 2016

TYsewyn commented Feb 20, 2016

TYsewyn commented Feb 25, 2016

rwinch commented Feb 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwinch commented Mar 1, 2016

Choose a reason for hiding this comment

TYsewyn commented Mar 1, 2016

Choose a reason for hiding this comment

rwinch commented Mar 2, 2016

TYsewyn commented Mar 4, 2016

rwinch commented Mar 4, 2016