Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

NEXUS-6323: Plexus cipher reimplementation #27

Closed
wants to merge 16 commits into
from

Conversation

Projects
None yet
4 participants
Contributor

cstamas commented Jul 17, 2014

Adding a re-implementation of PlexusCipher that uses CryptoHelper.

Issue
https://issues.sonatype.org/browse/NEXUS-6323

CI
NA

@cstamas cstamas referenced this pull request in sonatype/nexus-oss Jul 17, 2014

Merged

REVIEW NEXUS-6323: Replace plx cipher #636

@jdillon jdillon and 1 other commented on an outdated diff Jul 17, 2014

...su/goodies/crypto/internal/DefaultPasswordCipher.java
+ final @Named("${passwordCipher.algorithm:-PBEWithSHAAnd128BitRC4}") String algorithm,
+ final @Named("${passwordCipher.iterationCount:-23}") int iterationCount)
+ {
+ this.cryptoHelper = checkNotNull(cryptoHelper);
+ this.algorithm = checkNotNull(algorithm);
+ this.iterationCount = iterationCount;
+ this.base64Encoder = new Base64Encoder();
+ }
+
+ @Override
+ public String encrypt(final String str, final String passPhrase) {
+ checkNotNull(str);
+ checkNotNull(passPhrase);
+ try {
+ SecureRandom secureRandom = cryptoHelper.createSecureRandom();
+ secureRandom.setSeed(System.nanoTime());
@jdillon

jdillon Jul 17, 2014

Contributor

can probably avoid creating a new secure random each time, init this in ctor.

@cstamas

cstamas Jul 17, 2014

Contributor

Will do.

cstamas added some commits Jul 18, 2014

@cstamas cstamas NEXUS-6323: Implement plexus-cipher drop-in replacement
Also supporting legacy and current implementations, as
seemingly legacy is still used somewhere in NX.
48052f7
@cstamas cstamas NEXUS-6323: Undo POM change got in by mistake 829a8cf
@cstamas cstamas NEXUS-6323: The "current" should be default
To allow simple injection without need for @Named. Still,
"legaci" is injectable using provided name in iface.
f804024
@cstamas cstamas NEXUS-6323: Detect base64 problems and emit them as IAEx
As otherwise it would be a "generic" runtime ex wrapped by
Throwables, and reasoning about "bad input" would be lost.
afb1ca1

@mrprescott mrprescott and 1 other commented on an outdated diff Jul 22, 2014

.../sisu/goodies/crypto/internal/MavenCipherSupport.java
+import org.sonatype.sisu.goodies.common.ComponentSupport;
+import org.sonatype.sisu.goodies.crypto.MavenCipher;
+import org.sonatype.sisu.goodies.crypto.PasswordCipher;
+
+import com.google.common.base.Charsets;
+
+import com.google.common.base.Strings;
+import static com.google.common.base.Preconditions.checkArgument;
+import static com.google.common.base.Preconditions.checkNotNull;
+
+/**
+ * Support class for {@link MavenCipher} implementations.
+ *
+ * @since 1.10
+ */
+public abstract class MavenCipherSupport
@mrprescott

mrprescott Jul 22, 2014

Contributor

I'm probably missing something, but it seems this abstract class has two identical subclasses that don't add any functionality - could that be collapsed? (Or is the class name used as a lookup for something elsewhere?)

@mcculls

mcculls Jul 22, 2014

Member

The two subclasses have different injected constructor parameters, so while the base is the same they're configured differently (see PasswordCipherMavenLegacyImpl vs PasswordCipherMavenImpl which setup different algorithms).

@mrprescott mrprescott and 1 other commented on an outdated diff Jul 22, 2014

.../sisu/goodies/crypto/internal/MavenCipherSupport.java
+ }
+
+ protected String doDecrypt(final String str, final String passPhrase) {
+ return new String(passwordCipher.decrypt(str.getBytes(Charsets.UTF_8), passPhrase), Charsets.UTF_8);
+ }
+
+ @Override
+ public boolean isPasswordCipher(final String str) {
+ return peel(str) != null;
+ }
+
+ /**
+ * Peels of the start and stop "shield" braces from payload if possible, otherwise returns {@code null} signaling that
+ * input is invalid.
+ */
+ protected String peel(final String str) {
@cstamas

cstamas Jul 23, 2014

Contributor

Check.

Contributor

mrprescott commented Jul 22, 2014

+1

@jdillon jdillon and 1 other commented on an outdated diff Jul 22, 2014

...pe/sisu/goodies/crypto/internal/CryptoHelperImpl.java
@@ -143,7 +143,13 @@ public SecureRandom createSecureRandom() {
}
return obj;
}
-
+
+ @Override
+ public SecretKeyFactory createSecretKeyFactory(String transformation) throws NoSuchAlgorithmException {
+ checkNotNull(transformation);
+ return SecretKeyFactory.getInstance(transformation, getProvider());
+ }
@jdillon

jdillon Jul 22, 2014

Contributor

btw I've implemented this already on master, the impl here wasn't even following the same practice as the rest of the factory methods in this impl... did you not actually look to see what the others were doing?

@cstamas

cstamas Jul 23, 2014

Contributor

I had this change made on branch 5 days ago, but I did it only as I suspected that it belongs to CryptoHelper (and probably missed the part you ask about). Thanks for doing it on the master, will merge it.

cstamas and others added some commits Jul 23, 2014

Contributor

jdillon commented Jul 25, 2014

manually merged

@jdillon jdillon closed this Jul 25, 2014

@jdillon jdillon deleted the NEXUS-6323-plexus-cipher branch Jul 25, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment