[regression in 1.2 - vs 1.0] com.google.cloud.tools.jib.maven.DecryptedMavenSettings requires all servers to be decryptable

[rmannibucau]

Description of the issue:

in 1.0 only the used servers by the maven plugin was decrypted.
In 1.2, all are decrypted due to com.google.cloud.tools.jib.maven.DecryptedMavenSettings#from.

Expected behavior:

Get back 1.0 behavior.

Steps to reproduce:

Add a ciphered server not used by jib which can't be deciphered.

TIP: use an invalid password, in my case it was ${env.MISSING_VAR}

Environment:

Linux but probably any.

Log output:

Failed to execute goal com.google.cloud.tools:jib-maven-plugin:1.2.0:build (build) on project xxxx: Unable to decrypt settings.xml: [ERROR] Failed to decrypt password for server jetbrains: org.sonatype.plexus.components.sec.dispatcher.SecDispatcherException: java.io.FileNotFoundException: /home/travis/.m2/settings-security.xml (No such file or directory) @ server: jetbrains -> [Help 1]

(note: intentionally jib does not use settings.xml and it was working great in 1.0 but with 1.2 forced decryption makes the build failing, workaround is to pass decryption data but it is not desired)

[chanseokoh]

Thanks for the bug report.

I think we should turn this into a warning instead of failing if decryption fails. A missing environment variable like ${env.FORGOT_TO_SET} seems like a good example.

[rmannibucau]

@chanseokoh personnally I wouldnt even decrypt servers which are not used to ensure clear values are not even loaded. Do you think it can be an option?

[chanseokoh]

We also go through all the list of proxies to find active HTTP and HTTPS settings. Not failing on decryption failure will work as if nothing has been done for the failed decryption. It should just work, and I don't see any issue with it. What is your concern?

[loosebazooka]

Also decryptions appears to be happening wholesale on the file at the moment using the maven api. I'm not sure exactly if it's possible to do this per server.

[rmannibucau]

@chanseokoh having a build with warning can lead to a fail build on some CI platforms. One option can be to just ignore the failures but at the end there is never the need to decrypt something which is not used - which is safer by construction since we are speaking of passwords. Decryption can be done lazily when the server (same applies to proxy) is fetched injecting the component SettingsDecrypter as done here https://github.com/Talend/component-runtime/blob/87a387eb7796d11cab5ef4159035c54f1bbb3043/talend-component-maven-plugin/src/main/java/org/talend/sdk/component/maven/ImageM2Mojo.java#L195

[chanseokoh]

I get what you're saying, but here's my argument.

(same applies to proxy)

If <proxies> are configured, all HTTP/HTTPS requests and responses should go through the proxies, so you'll end up having to decrypt the whole settings.xml anyways. Also, given that Maven API does the wholesale decryption and doesn't allow selectively decrypting individual passwords, I think there is no real concern about decrypting something unused. Sure, you could argue that if <proxies> have not ever been defined or there's no <password> for proxies, lazy decryption for <servers> may make a difference, but given how Maven decrypts it, it doesn't seem worth another branch fork in our code.

having a build with warning can lead to a fail build on some CI platforms.

This is a rare situation in that you had ${env.FORGOT_TO_SET} in settings.xml and you forgot to define the said environment variable, and it may actually be better to point that out, especially for CI builds where people often hit a lot of instances of failing to inject environment variables. Even if CI fails on warning, it's also an easy fix by setting the missing env variable.

[rmannibucau]

@chanseokoh I'm not following:

1. maven does not enforce to decrypt the settings as a whole - it is actually rare in plugins. Check the API I linked - org.apache.maven.settings.crypto.SettingsDecrypter, it is a maven one,
2. this is not because you go through the proxies (list) that you have to decrypt them all, if you check out your ProxyProvider, you actually only use a single proxy (findFirst)

So in all cases a lazy decryption is saner IMO.

About the "rare situation", it depends your settings.xml, locally I have ~20 credentials there and per project I tend to use 3-4 only. On the CI all PR builds will have this MISSING_VAR - not FORGOTTEN ;) - setup since part of the variables are only for master build - like documentation redeployment.

[chanseokoh]

Ah, sorry. I didn't look into the Maven API actually. Seems like you can decrypt per server.

[loosebazooka]

@rmannibucau, you're right, the API does allow it. We actually changed the code to wholesale decryption based on some weird code paths that we had (sharing gradle/maven code). We'll take another look at this to see if we can solve it more gracefully.

[rmannibucau]

if it helps: a trivial way to solve it is to copy the settings (really new X().set(original.get()) pattern) and use a DecryptableServer instance extending Server which does the decryption lazily (you pass the decrypter in the constructor). It limits a lot the impacts even if not super elegant. That said happy with any solution on my side :)

[chanseokoh]

Fixed by #1712. Closing.

[chanseokoh]

@rmannibucau v1.3.0 is released, and it will not decrypt passwords that are only necessary on demand.