REVIEW: Enhanced password hashing #802

Merged
merged 20 commits into from May 3, 2013

Conversation

Projects
None yet
3 participants
Contributor

scarlucci commented Apr 5, 2013

These changes are for https://issues.sonatype.org/browse/NXCM-4361. There are corresponding changes for this work in sonatype/security

The goal of the changes is to use a more secure mechanism to hash user passwords. The changes leverage the new PasswordService feature in Shiro 1.2 to encapsulate all of the logic related to how we hash passwords.

Salting:
Passwords are now salted prior to hashing. Salts are unique per user, per password.

Hashing algorithm:
Passwords are now hashed with SHA-512. Previously, passwords were hashed with SHA-1. The password matcher is backwards compatible with SHA-1 and MD5 hashes

Key Stretching:
The hashing algorithm is now applied N times, where N is defined in security-configuration.xml, in the new hashIterations element. The model version of security-configuration.xml has been updated.

Upgrading legacy users:
In order to convert old SHA-1 password hashes, we must wait for the user to login. When they do, we detect that their password hash is old and regenerate the hash

scarlucci closed this Apr 12, 2013

scarlucci reopened this May 3, 2013

Contributor

ifedorenko commented May 3, 2013

+1

@kellyrob99 kellyrob99 and 1 other commented on an outdated diff May 3, 2013

...ting/internal/TextFilePrefixSourceMarshallerTest.java
@@ -143,7 +143,7 @@ public void roundtrip()
assertThat( outputStream.size(), greaterThan( 15 ) );
final String output = new String( outputStream.toByteArray(), UTF8 );
- assertThat( output, equalTo( withStandardHeaders( prefixFile1( false ) ) ) );
+ assertThat( output.replace("\r", ""), equalTo( withStandardHeaders( prefixFile1( false ) ).replace("\r", "") ) );
@kellyrob99

kellyrob99 May 3, 2013

Owner

Any special reason we have to strip carriage returns now?

@scarlucci

scarlucci May 3, 2013

Contributor

This change is obsolete now. I had fixed this awhile ago on this branch, but ended up fixing on master in a better way. I will remove this change

@kellyrob99 kellyrob99 commented on the diff May 3, 2013

...ype/nexus/web/PlexusContainerContextListenerTest.java
@@ -1,66 +0,0 @@
-/*
@kellyrob99

kellyrob99 May 3, 2013

Owner

This doesn't have any relevance to the task at hand, does it?

@scarlucci

scarlucci May 3, 2013

Contributor

This test was going to need some updates to work with these changes, and the comments in the test suggested it was obsolete. Before spending any time on this test, I spoke with Tamas, who said to kill it

Owner

kellyrob99 commented May 3, 2013

Couple of minor questions, but nothing that should hold up merging
+1

@scarlucci scarlucci Undo change to get test working on Windows
This change was already made on master, in a better way
04ec5db

@scarlucci scarlucci added a commit that referenced this pull request May 3, 2013

@scarlucci scarlucci Merge pull request #802 from sonatype/NXCM-4361-salt-rework
REVIEW: Enhanced password hashing
7829bfc

@scarlucci scarlucci merged commit 7829bfc into master May 3, 2013

scarlucci deleted the NXCM-4361-salt-rework branch May 3, 2013

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