Apply Maven SettingsDecrypter for encrypted server details #609

briandealwis · 2018-07-13T18:00:25Z

My assumption here is that a decryption error in MavenSettingsServerCredentials should fail the build rather than either return the still-encrypted password or return null (i.e., no configured credentials).

Unsure about MavenSettingsServerCredentials throwing a MojoExecutionException directly, but it seemed unhelpful to create a new exception class that would be immediately turned into a MojoExecutionException.

[ERROR] Failed to execute goal com.google.cloud.tools:jib-maven-plugin:0.9.7-SNAPSHOT:build (default-deploy) on project first: Unable to decrypt settings for registry.hub.docker.com: [ERROR] Failed to decrypt password for server registry.hub.docker.com: org.sonatype.plexus.components.sec.dispatcher.SecDispatcherException: java.io.FileNotFoundException: /Users/bsd/.m2/settings-security.xml (No such file or directory) @ server: registry.hub.docker.com -> [Help 1]

Fixes #592

chanseokoh · 2018-07-13T18:21:29Z

...en-plugin/src/main/java/com/google/cloud/tools/jib/maven/MavenSettingsServerCredentials.java

+        if (problem.getSeverity() == SettingsProblem.Severity.ERROR
+            || problem.getSeverity() == SettingsProblem.Severity.FATAL) {
+          throw new MojoExecutionException(
+              "Unable to decrypt settings for " + registry + ": " + problem);


I think "... decrypt password for ..." is more informative to actual users and has the potential to make them actions on their own.

eddiewebb · 2018-07-13T18:28:22Z

Looks like you missed updating method call in BuildTarMojo to pass decrypter. But fixed that and built locally and confirm it does indeed decrypt and publish 🎉

coollog

Nice! Just some small comments.

coollog · 2018-07-13T19:48:31Z

jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/BuildDockerMojo.java

@@ -44,7 +44,6 @@
    name = BuildDockerMojo.GOAL_NAME,
    requiresDependencyResolution = ResolutionScope.RUNTIME_PLUS_SYSTEM)
 public class BuildDockerMojo extends JibPluginConfiguration {
-


nit: a line removed (we're following a convention of empty line after class definition for this codebase)

coollog · 2018-07-13T19:51:13Z

jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/JibPluginConfiguration.java

@@ -164,6 +166,8 @@ void handleDeprecatedParameters(BuildLogger logger) {
  @Parameter(defaultValue = "${project.basedir}/src/main/jib", required = true)
  private String extraDirectory;

+  @Nullable @Component protected SettingsDecrypter settingsDecrypter;


Would this ever be null?

I'm not sure. nullaway complains without it. I did some searching in the Maven docs but didn't find anything authoritative.

The Github Maven Plugins look up the SettingsDecrypter through the plexus container and guard against a ComponentLookupException.

Just a native question, is it possible fix encrypted passwords and plain passwords (for different servers) in settings.xml? If that's true, I just want to make sure settingsDecrypter handles plain passwords nicely. (I do believe it will.)

Perhaps we should do that as well. The case we should guard against is if the user has an encrypted password and SettingsDecrypter was null, there should be some error rather than trying to use the encrypted password and failing to authenticate (by which the user thinks the password is wrong but that's not the case)

I think @chanseokoh is asking about ~/.m2/settings.xml having both encrypted and unencrypted passwords. The default SettingsDecryptor passes-through unencrypted passwords.

The SettingsDecrypter could theoretically be replaced. In the worse case, where the SettingsDecrypter is null but we have encrypted passwords, we will attempt to authenticate with the encrypted password bytes. But since it was encrypted we won't have divulged anything in the clear. There's no solid way to identity an encrypted vs unencrypted password though.

Mmhmm, I was mostly trying to get at the user experience side, like:

User sets encrypted password in Maven settings

We try to decrypt, but SettingsDecrypter is null, so the password wasn't decrypted

Users sees 401 Unauthorized and believes their password is wrong

Whereas, I think the better UX would be:

User sets encrypted password in Maven settings

We try to decrypt, but SettingsDecrypter is null, so the password wasn't decrypted, and we give an error

Users sees failed to decrypt password (or similar)

The default encryption approach seems to surround the passwords with braces.

We could pass in the mavenBuildLogger here and if settingsDecrypter == null and the password starts and ends with braces then issue a warning.

In MavenSettingsServerCredentials#retrieve right? I might be misunderstanding how SettingsDecrypter works, but is there any harm in giving the warning just if settingsDecrypter == null?

coollog · 2018-07-13T19:52:30Z

...en-plugin/src/main/java/com/google/cloud/tools/jib/maven/MavenSettingsServerCredentials.java

+      }
+      if (result.getServer() != null) {
+        registryServer = result.getServer();
+      }


If it is null, should we error?

SettingsDecrypter and SettingsDecryptionResult are under-documented and does not discuss the possible meanings of the return results. SettingsDecryptionResult#getServers() does note that:

/** * Gets the decrypted servers. * * @return The decrypted server, can be empty but never {@code null}. */ List<Server> getServers();

and the implementation of getServer() returns the first element from getServers() and null if there are no servers.

By this point, we have no errors reported from the decryption request, but we have no servers. So one interpretation is: no decryption was required, so use the original server.

Makes sense! Let's maybe add a comment about the null case meaning no decryption was required.

eddiewebb · 2018-07-14T01:11:12Z

Just a note @briandealwis, according to Maven docs it is valid to put notes inside the password element, but outside the curly braces. So it is not a safe assumption that encrypted values will start with {

<password>Eddie reset this on 2018-07-13 {hsg36sheyeue6duduesy}</password>.

So you'd have to anticipate that in any 'is it encrypted' logic

coollog

Looks good! Just a style comment.

coollog · 2018-07-14T05:47:46Z

...en-plugin/src/main/java/com/google/cloud/tools/jib/maven/MavenSettingsServerCredentials.java

+
+  // pattern cribbed directly from
+  // https://github.com/sonatype/plexus-cipher/blob/master/src/main/java/org/sonatype/plexus/components/cipher/DefaultPlexusCipher.java
+  private static final Pattern ENCRYPTED_STRING_PATTERN =


In this codebase, we're following a convention of organizing classes in the order:

static->non-static

members->inner class->constructor->methods

public->private

final->nonfinal

So a ENCRYPTED_STRING_PATTERN would be under CREDENTIAL_SOURCE, isEncrypted would be under ENCRYPTED_STRING_PATTERN.

coollog · 2018-07-15T03:48:56Z

Oh I forgot to mention to include the CHANGELOG entry too.

briandealwis added 2 commits July 13, 2018 11:22

Remove unthrown exception

8885deb

Apply Maven SettingsDecrypter to server settings

a4cf218

briandealwis added the kokoro:run label Jul 13, 2018

kokoro-team removed the kokoro:run label Jul 13, 2018

Merge branch 'master' into i592

ded346e

briandealwis added the kokoro:run label Jul 13, 2018

kokoro-team removed the kokoro:run label Jul 13, 2018

chanseokoh reviewed Jul 13, 2018

View reviewed changes

briandealwis added 2 commits July 13, 2018 14:36

Tweak wording, and fixup :buildTar

4b9bd04

Make nullaway happy

d0e676a

coollog added this to the v0.9.7 milestone Jul 13, 2018

briandealwis added the kokoro:run label Jul 13, 2018

kokoro-team removed kokoro:run labels Jul 13, 2018

Merge branch 'master' into i592

cf6e633

coollog reviewed Jul 13, 2018

View reviewed changes

briandealwis added 2 commits July 13, 2018 16:20

Restore removed blank line.

f4faf63

document treatment of SettingsDecryptionResult

25d4763

briandealwis added 2 commits July 14, 2018 01:38

Warn on encrypted passwords when no decrypter is available

043387d

Merge remote-tracking branch 'origin/master' into HEAD

c5a8aa6

coollog reviewed Jul 14, 2018

View reviewed changes

sort additions

a635ab2

coollog approved these changes Jul 14, 2018

View reviewed changes

briandealwis merged commit f065c30 into master Jul 15, 2018

briandealwis deleted the i592 branch July 15, 2018 02:43

chanseokoh mentioned this pull request Jul 16, 2018

Build image failed #544

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Apply Maven SettingsDecrypter for encrypted server details #609

Apply Maven SettingsDecrypter for encrypted server details #609

briandealwis commented Jul 13, 2018

chanseokoh Jul 13, 2018

eddiewebb commented Jul 13, 2018 •

edited

Loading

coollog left a comment

coollog Jul 13, 2018

coollog Jul 13, 2018

briandealwis Jul 13, 2018

chanseokoh Jul 13, 2018

coollog Jul 13, 2018

briandealwis Jul 13, 2018

coollog Jul 13, 2018

briandealwis Jul 13, 2018

coollog Jul 13, 2018

coollog Jul 13, 2018

briandealwis Jul 13, 2018

coollog Jul 13, 2018

eddiewebb commented Jul 14, 2018

coollog left a comment

coollog Jul 14, 2018

coollog commented Jul 15, 2018

Apply Maven SettingsDecrypter for encrypted server details #609

Apply Maven SettingsDecrypter for encrypted server details #609

Conversation

briandealwis commented Jul 13, 2018

Choose a reason for hiding this comment

eddiewebb commented Jul 13, 2018 • edited Loading

coollog left a 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

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

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

eddiewebb commented Jul 14, 2018

coollog left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coollog commented Jul 15, 2018

eddiewebb commented Jul 13, 2018 •

edited

Loading