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

[MNG-5583] per endpoint support for PKI authentication #67

Closed
wants to merge 7 commits into from
Closed

[MNG-5583] per endpoint support for PKI authentication #67

wants to merge 7 commits into from

Conversation

spyhunter99
Copy link

@spyhunter99 spyhunter99 commented May 18, 2020

This change provides some additional functionality for authenticating to a maven repository that requires HTTP Client Certificate authentication as part of the SSL hand shake.

see also apache/maven-resolver#51
apache/maven#345

*
*
*/
private String trustStorePassword;

Choose a reason for hiding this comment

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

This field contains a password. We must convert it to char[] later on, so can't we store it as a char[]?

Copy link
Member

Choose a reason for hiding this comment

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

The upstream model does not allow that. Given that other password fields are strings too, this is acceptable, but not ideal.

Copy link
Author

Choose a reason for hiding this comment

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

i agree with @michael-o otherwise i would have used char[]

*
* .
*/
private String keyStorePassword;

Choose a reason for hiding this comment

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

This field contains a password. We must convert it to char[] later on, so can't we store it as a char[]?

alias = null;
}
}
else if ( keystore.containsAlias( alias.toLowerCase() ) )

Choose a reason for hiding this comment

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

Why this additional check on the lower-cased variant?

Copy link
Author

Choose a reason for hiding this comment

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

this was borrowed from tomcat's code base, apparently in certain situational, key aliases are case folded. i've never seen it in the wild either, it's always been case sensitive but i figured there was a reason for this. lmk if you want it stricken from the record

Copy link
Author

Choose a reason for hiding this comment

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

*
*
*/
private String trustStorePassword;
Copy link
Member

Choose a reason for hiding this comment

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

The upstream model does not allow that. Given that other password fields are strings too, this is acceptable, but not ideal.

@spyhunter99
Copy link
Author

FYI i'm going to try and add some new unit tests for this to have a more reproducible setup rather than the hacked up sonatype nexus server i'm testing against

@spyhunter99
Copy link
Author

regarding unit tests, whats the difference between the regular http wagon and the light weight version?

@michael-o
Copy link
Member

regarding unit tests, whats the difference between the regular http wagon and the light weight version?

Bad naming. HttpWagon uses Apache HttpClient, LightweightClient uses HttpURLConnection.

@spyhunter99
Copy link
Author

regarding unit tests, whats the difference between the regular http wagon and the light weight version?

Bad naming. HttpWagon uses Apache HttpClient, LightweightClient uses HttpURLConnection.

perhaps i asked the wrong question. In what context is the lightweight version used vs the httpwagon version? i'm trying to figure out if i need to (or should) add the pki support mechanisms to the lightweight setup

@michael-o
Copy link
Member

regarding unit tests, whats the difference between the regular http wagon and the light weight version?

Bad naming. HttpWagon uses Apache HttpClient, LightweightClient uses HttpURLConnection.

perhaps i asked the wrong question. In what context is the lightweight version used vs the httpwagon version? i'm trying to figure out if i need to (or should) add the pki support mechanisms to the lightweight setup

Not in Maven itself. It is a user desicion to swap the HTTP client backend for Wagon. The LightweightWagon has flaws as documented in JIRA.

@spyhunter99
Copy link
Author

anything else that needs to changed on this PR?

import javax.net.ssl.X509KeyManager;

/**
* This was borrowed from Apache Tomcat

Choose a reason for hiding this comment

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

Does Tomcat also have a unit test for this class? If so, I think we might want to borrow that as well.

Copy link
Author

Choose a reason for hiding this comment

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

Doesn't look like it. I'll see if I can whip one up. The purpose of the class is to force a selection for a specific certificate. The default JRE implementation will pick the first key pair that matches the server's criteria as part of the hand shake. If you have more than one that matches the criteria but only a specific one will allow access, you're hosed.

Copy link
Author

Choose a reason for hiding this comment

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

actually i'm glad you brought this up, definitely needs a test case. I shouldn't have copied tomcat's code without reading the details. It's purpose is to select the server certificate to for when a single tomcat server hosts multiple domains all in the same key store. We actually need the opposite, client certificate selection.

@michael-o
Copy link
Member

Since this is a massive change, I have to complete the current Wagon release first and then will pick this up in June.

@elharo elharo changed the title Feature/MNG-5583 per endpoint support for PKI authentication [MNG-5583] per endpoint support for PKI authentication Feb 18, 2021
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants