You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by GitBox <gi...@apache.org> on 2020/05/18 22:28:36 UTC
[GitHub] [maven-resolver] spyhunter99 opened a new pull request #51: MNG-5583 initial commit for supporting PKI authentication to nexus re…
spyhunter99 opened a new pull request #51:
URL: https://github.com/apache/maven-resolver/pull/51
…positories. also requires changes to maven-wagon
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] michael-o commented on a change in pull request #51: MNG-5583 per endpoint support for PKI authentication to maven repos
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #51:
URL: https://github.com/apache/maven-resolver/pull/51#discussion_r427600652
##########
File path: maven-resolver-transport-wagon/pom.xml
##########
@@ -56,7 +56,7 @@
<dependency>
<groupId>org.apache.maven.wagon</groupId>
<artifactId>wagon-provider-api</artifactId>
- <version>3.4.0</version>
+ <version>3.4.1-SNAPSHOT</version>
Review comment:
This is not possible, we don't depend on snapshots
##########
File path: maven-resolver-api/src/main/java/org/eclipse/aether/repository/AuthenticationContext.java
##########
@@ -272,6 +272,19 @@ public String get( String key )
{
fillingAuthData = true;
auth.fill( this, key, data );
+
+ //MNG-5583 per endpoint PKI authentication
Review comment:
I don't understand the purpose of this hunk
##########
File path: maven-resolver-api/src/test/java/org/eclipse/aether/repository/AuthenticationContextTest.java
##########
@@ -56,7 +56,7 @@ public void fill( AuthenticationContext context, String key, Map<String, String>
assertNotNull( context );
assertNotNull( context.getSession() );
assertNotNull( context.getRepository() );
- assertNull( "fill() should only be called once", context.get( "key" ) );
+ //assertNull( "fill() should only be called once", context.get( "key" ) );
Review comment:
Agree here
##########
File path: maven-resolver-transport-wagon/src/main/java/org/eclipse/aether/transport/wagon/WagonTransporter.java
##########
@@ -185,32 +184,21 @@ private AuthenticationInfo getAuthenticationInfo( RemoteRepository repository,
if ( authContext != null )
{
- auth = new AuthenticationInfo()
- {
- @Override
- public String getUserName()
- {
- return authContext.get( AuthenticationContext.USERNAME );
- }
-
- @Override
- public String getPassword()
- {
- return authContext.get( AuthenticationContext.PASSWORD );
- }
-
- @Override
- public String getPrivateKey()
- {
- return authContext.get( AuthenticationContext.PRIVATE_KEY_PATH );
- }
-
- @Override
- public String getPassphrase()
- {
- return authContext.get( AuthenticationContext.PRIVATE_KEY_PASSPHRASE );
- }
- };
+ auth = new AuthenticationInfo();
+ auth.setUserName( authContext.get( AuthenticationContext.USERNAME ) );
+ auth.setPassword( authContext.get( AuthenticationContext.PASSWORD ) );
+ auth.setPrivateKey( authContext.get( AuthenticationContext.PRIVATE_KEY_PATH ) );
+ auth.setPassphrase( authContext.get( AuthenticationContext.PRIVATE_KEY_PASSPHRASE ) );
+ //MNG-5583 per endpoint PKI authentication
Review comment:
Redundant comment = >commit message
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] olamy commented on a change in pull request #51: MNG-5583 per endpoint support for PKI authentication to maven repos
Posted by GitBox <gi...@apache.org>.
olamy commented on a change in pull request #51:
URL: https://github.com/apache/maven-resolver/pull/51#discussion_r427743790
##########
File path: maven-resolver-transport-wagon/pom.xml
##########
@@ -56,7 +56,7 @@
<dependency>
<groupId>org.apache.maven.wagon</groupId>
<artifactId>wagon-provider-api</artifactId>
- <version>3.4.0</version>
+ <version>3.4.1-SNAPSHOT</version>
Review comment:
I don't understand the problem??
This sounds complicated to not depends on a SNAPSHOT?? at least until we did enough testing and wagon get released.
I'm totally fine as it will be continuous testing as master of our projects are deployed as SNAPSHOT for every commits.
How do you expect to build/test everything together in CI system???
Or even how do you except reviewing this PR??
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] michael-o commented on a change in pull request #51: MNG-5583 per endpoint support for PKI authentication to maven repos
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #51:
URL: https://github.com/apache/maven-resolver/pull/51#discussion_r427807001
##########
File path: maven-resolver-transport-wagon/pom.xml
##########
@@ -56,7 +56,7 @@
<dependency>
<groupId>org.apache.maven.wagon</groupId>
<artifactId>wagon-provider-api</artifactId>
- <version>3.4.0</version>
+ <version>3.4.1-SNAPSHOT</version>
Review comment:
@olamy I think we have communicated at all times that master must be buildable. With a snapshot it is not. There is no guarantee that the snapshot will be available or available in a compatible form.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] spyhunter99 commented on a change in pull request #51: MNG-5583 per endpoint support for PKI authentication to maven repos
Posted by GitBox <gi...@apache.org>.
spyhunter99 commented on a change in pull request #51:
URL: https://github.com/apache/maven-resolver/pull/51#discussion_r427653434
##########
File path: maven-resolver-api/src/main/java/org/eclipse/aether/repository/AuthenticationContext.java
##########
@@ -272,6 +272,19 @@ public String get( String key )
{
fillingAuthData = true;
auth.fill( this, key, data );
+
+ //MNG-5583 per endpoint PKI authentication
Review comment:
this hunk is also the reason why i disabled that one assert in the unit test. There's something strange going on and i was unable to step debug it nor understand what the scope and source was, but the symptoms are that at some point, a new AuthenticationInfo class was being created and it was only carrying over the username and password, not the rest of the settings. Drove me up the wall for a few hours. I was hoping someone else knew what the source of this behavior was but i couldn't find it
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] spyhunter99 commented on a change in pull request #51: MNG-5583 per endpoint support for PKI authentication to maven repos
Posted by GitBox <gi...@apache.org>.
spyhunter99 commented on a change in pull request #51:
URL: https://github.com/apache/maven-resolver/pull/51#discussion_r427653569
##########
File path: maven-resolver-transport-wagon/pom.xml
##########
@@ -56,7 +56,7 @@
<dependency>
<groupId>org.apache.maven.wagon</groupId>
<artifactId>wagon-provider-api</artifactId>
- <version>3.4.0</version>
+ <version>3.4.1-SNAPSHOT</version>
Review comment:
understood,i'll update it, assuming the wagon PR gets merged and released
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] olamy commented on a change in pull request #51: MNG-5583 per endpoint support for PKI authentication to maven repos
Posted by GitBox <gi...@apache.org>.
olamy commented on a change in pull request #51:
URL: https://github.com/apache/maven-resolver/pull/51#discussion_r427743790
##########
File path: maven-resolver-transport-wagon/pom.xml
##########
@@ -56,7 +56,7 @@
<dependency>
<groupId>org.apache.maven.wagon</groupId>
<artifactId>wagon-provider-api</artifactId>
- <version>3.4.0</version>
+ <version>3.4.1-SNAPSHOT</version>
Review comment:
I don't understand the problem??
This sounds complicated to not depends on a SNAPSHOT?? at least until we did enough testing and wagon get released.
I'm totally fine as it will be continuous testing as master of our projects are deployed as SNAPSHOT for every commits.
How do you expect to build/test everything together in CI system???
Or even how do you except reviewing this PR??
we will just mark this as dependant of wagon SNAPSHOT if we accept both for sure.
At least it makes it easy to test it locally because here we have an issue which need to build 3 projects before being able to test everything. So yes using SNAPSHOT will make it easier
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] spyhunter99 commented on a change in pull request #51: MNG-5583 per endpoint support for PKI authentication to maven repos
Posted by GitBox <gi...@apache.org>.
spyhunter99 commented on a change in pull request #51:
URL: https://github.com/apache/maven-resolver/pull/51#discussion_r427652722
##########
File path: maven-resolver-transport-wagon/src/main/java/org/eclipse/aether/transport/wagon/WagonTransporter.java
##########
@@ -185,32 +184,21 @@ private AuthenticationInfo getAuthenticationInfo( RemoteRepository repository,
if ( authContext != null )
{
- auth = new AuthenticationInfo()
- {
- @Override
- public String getUserName()
- {
- return authContext.get( AuthenticationContext.USERNAME );
- }
-
- @Override
- public String getPassword()
- {
- return authContext.get( AuthenticationContext.PASSWORD );
- }
-
- @Override
- public String getPrivateKey()
- {
- return authContext.get( AuthenticationContext.PRIVATE_KEY_PATH );
- }
-
- @Override
- public String getPassphrase()
- {
- return authContext.get( AuthenticationContext.PRIVATE_KEY_PASSPHRASE );
- }
- };
+ auth = new AuthenticationInfo();
+ auth.setUserName( authContext.get( AuthenticationContext.USERNAME ) );
+ auth.setPassword( authContext.get( AuthenticationContext.PASSWORD ) );
+ auth.setPrivateKey( authContext.get( AuthenticationContext.PRIVATE_KEY_PATH ) );
+ auth.setPassphrase( authContext.get( AuthenticationContext.PRIVATE_KEY_PASSPHRASE ) );
+ //MNG-5583 per endpoint PKI authentication
+ auth.setKeyAlias( authContext.get( "getKeyAlias" ) );
Review comment:
see the other PR for maven core. I was a bit hesitant on this too but i figured someone else with more knowledge that me would chime in with the right answer. I can add these keys in the AuthenticationContext class as constants, would that work?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] olamy commented on a change in pull request #51: MNG-5583 per endpoint support for PKI authentication to maven repos
Posted by GitBox <gi...@apache.org>.
olamy commented on a change in pull request #51:
URL: https://github.com/apache/maven-resolver/pull/51#discussion_r427855452
##########
File path: maven-resolver-transport-wagon/pom.xml
##########
@@ -56,7 +56,7 @@
<dependency>
<groupId>org.apache.maven.wagon</groupId>
<artifactId>wagon-provider-api</artifactId>
- <version>3.4.0</version>
+ <version>3.4.1-SNAPSHOT</version>
Review comment:
as long as the PR has been merged in wagon I do not see any issues with that.. we are NOT 100 developers who keep deploying snapshots :) and it's easier to have this SNAPSHOT dependencies for testing this PR and even within an IDE as they are integrating such SNAPSHOT dependencies and can link projects together which is definitely so easier for development purpose. Always good to ease contributors life
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] mthmulders commented on a change in pull request #51: MNG-5583 per endpoint support for PKI authentication to maven repos
Posted by GitBox <gi...@apache.org>.
mthmulders commented on a change in pull request #51:
URL: https://github.com/apache/maven-resolver/pull/51#discussion_r427053009
##########
File path: maven-resolver-api/src/test/java/org/eclipse/aether/repository/AuthenticationContextTest.java
##########
@@ -56,7 +56,7 @@ public void fill( AuthenticationContext context, String key, Map<String, String>
assertNotNull( context );
assertNotNull( context.getSession() );
assertNotNull( context.getRepository() );
- assertNull( "fill() should only be called once", context.get( "key" ) );
+ //assertNull( "fill() should only be called once", context.get( "key" ) );
Review comment:
I don't feel like we should disable assertions by commenting them. Also, I don't feel like we should disable assertions at all.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] olamy commented on a change in pull request #51: MNG-5583 per endpoint support for PKI authentication to maven repos
Posted by GitBox <gi...@apache.org>.
olamy commented on a change in pull request #51:
URL: https://github.com/apache/maven-resolver/pull/51#discussion_r427855452
##########
File path: maven-resolver-transport-wagon/pom.xml
##########
@@ -56,7 +56,7 @@
<dependency>
<groupId>org.apache.maven.wagon</groupId>
<artifactId>wagon-provider-api</artifactId>
- <version>3.4.0</version>
+ <version>3.4.1-SNAPSHOT</version>
Review comment:
as long as the PR has been merged in wagon I do not see any issues with that.. we are 100 developers who keep deploying snapshots :) and it's easier to have this SNAPSHOT dependencies for testing this PR and even within an IDE as they are integrating such SNAPSHOT and can link projects together which is definitely so easier for development purpose. Always good to ease contributors life
##########
File path: maven-resolver-transport-wagon/pom.xml
##########
@@ -56,7 +56,7 @@
<dependency>
<groupId>org.apache.maven.wagon</groupId>
<artifactId>wagon-provider-api</artifactId>
- <version>3.4.0</version>
+ <version>3.4.1-SNAPSHOT</version>
Review comment:
as long as the PR has been merged in wagon I do not see any issues with that.. we are NOT 100 developers who keep deploying snapshots :) and it's easier to have this SNAPSHOT dependencies for testing this PR and even within an IDE as they are integrating such SNAPSHOT and can link projects together which is definitely so easier for development purpose. Always good to ease contributors life
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] mthmulders commented on a change in pull request #51: MNG-5583 per endpoint support for PKI authentication to maven repos
Posted by GitBox <gi...@apache.org>.
mthmulders commented on a change in pull request #51:
URL: https://github.com/apache/maven-resolver/pull/51#discussion_r427057175
##########
File path: maven-resolver-transport-wagon/src/main/java/org/eclipse/aether/transport/wagon/WagonTransporter.java
##########
@@ -185,32 +184,21 @@ private AuthenticationInfo getAuthenticationInfo( RemoteRepository repository,
if ( authContext != null )
{
- auth = new AuthenticationInfo()
- {
- @Override
- public String getUserName()
- {
- return authContext.get( AuthenticationContext.USERNAME );
- }
-
- @Override
- public String getPassword()
- {
- return authContext.get( AuthenticationContext.PASSWORD );
- }
-
- @Override
- public String getPrivateKey()
- {
- return authContext.get( AuthenticationContext.PRIVATE_KEY_PATH );
- }
-
- @Override
- public String getPassphrase()
- {
- return authContext.get( AuthenticationContext.PRIVATE_KEY_PASSPHRASE );
- }
- };
+ auth = new AuthenticationInfo();
Review comment:
I'm glad to see we no longer create an anonymous subtype of `AuthenticationInfo` :-)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] mthmulders commented on a change in pull request #51: MNG-5583 per endpoint support for PKI authentication to maven repos
Posted by GitBox <gi...@apache.org>.
mthmulders commented on a change in pull request #51:
URL: https://github.com/apache/maven-resolver/pull/51#discussion_r427057035
##########
File path: maven-resolver-transport-wagon/src/main/java/org/eclipse/aether/transport/wagon/WagonTransporter.java
##########
@@ -185,32 +184,21 @@ private AuthenticationInfo getAuthenticationInfo( RemoteRepository repository,
if ( authContext != null )
{
- auth = new AuthenticationInfo()
- {
- @Override
- public String getUserName()
- {
- return authContext.get( AuthenticationContext.USERNAME );
- }
-
- @Override
- public String getPassword()
- {
- return authContext.get( AuthenticationContext.PASSWORD );
- }
-
- @Override
- public String getPrivateKey()
- {
- return authContext.get( AuthenticationContext.PRIVATE_KEY_PATH );
- }
-
- @Override
- public String getPassphrase()
- {
- return authContext.get( AuthenticationContext.PRIVATE_KEY_PASSPHRASE );
- }
- };
+ auth = new AuthenticationInfo();
+ auth.setUserName( authContext.get( AuthenticationContext.USERNAME ) );
+ auth.setPassword( authContext.get( AuthenticationContext.PASSWORD ) );
+ auth.setPrivateKey( authContext.get( AuthenticationContext.PRIVATE_KEY_PATH ) );
+ auth.setPassphrase( authContext.get( AuthenticationContext.PRIVATE_KEY_PASSPHRASE ) );
+ //MNG-5583 per endpoint PKI authentication
+ auth.setKeyAlias( authContext.get( "getKeyAlias" ) );
Review comment:
The above keys are pre-defined constants in Aether, and so we may assume they correspond to existing data in the `AuthenticationContext`. The keys below have a different naming structure (they all start with `get`) and I'm not sure how the data we're looking for actually ended up there?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org