Skip to content
This repository has been archived by the owner on Feb 8, 2019. It is now read-only.

Fixes #8593: UserManagement need to have hashed password for both Linux and AIX #84

Merged
merged 1 commit into from
Jul 19, 2016
Merged

Fixes #8593: UserManagement need to have hashed password for both Linux and AIX #84

merged 1 commit into from
Jul 19, 2016

Conversation

fanf
Copy link
Member

@fanf fanf commented Jun 30, 2016 •
edited
Loading

https://www.rudder-project.org/redmine/issues/8593

The diff is better viewed with ?w=1, because big parts of the code were reindented.
The three main parts are:

  • AixPasswordHashAlgos (and its tests): implements the correct hash algo for AIX smd5, ssha1, ssha256 and ssh512. The last two need PBKDF2WithHmacSHA256 / PBKDF2WithHmacSHA512, and so Java 8.
  • add a new <CONSTRAINT> elements, <USES>, that instructs the input to do things with the other inputs in <USES>
  • all other things: add two new input type, masterPassword and slavePassword:aix, that will be mapped to two new fields in directive editor (masterPassword looks like the current password, and slavePassword doesn't display anything).

@fanf fanf force-pushed the bug_8593/usermanagement_need_to_have_hashed_password_for_both_linux_and_aix branch from a4e2484 to af4d6ac Compare June 30, 2016 15:12
@fanf
Copy link
Member Author

fanf commented Jun 30, 2016

PR rebased

ncharles
ncharles reviewed Jun 30, 2016
View reviewed changes
src/main/scala/com/normation/cfclerk/domain/AixPasswordHashAlgo.scala
val rounds = 2 << (cost-1)
val s = salt.getOrElse(getRandomSalt(16)).getBytes("UTF-8")
val spec: PBEKeySpec = new PBEKeySpec(pwd.toCharArray, s, rounds, 8*sha.byteNumber)
val skf: SecretKeyFactory = SecretKeyFactory.getInstance(s"PBKDF2WithHmac${sha.name}")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can't that fail ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ha, this is in a global try/catch

@ncharles
Copy link
Member

Looks great. I'd really appreciate more comments on it though

ncharles
ncharles reviewed Jul 5, 2016
View reviewed changes
src/main/scala/com/normation/cfclerk/xmlparsers/CfclerkXmlConstants.scala
@@ -118,5 +118,6 @@ object CfclerkXmlConstants {
val CONSTRAINT_DEFAULT = "DEFAULT"
val CONSTRAINT_REGEX = "REGEX"
val CONSTRAINT_PASSWORD_HASH = "PASSWORDHASH"
val CONSTRAINT_USED_FIELDS = "USES"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CONSTRAINT_USED_FIELDS -> CONSTRAINT_USES_FIELDS ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe it should be USED here.
In fact, USES is a terrible keywork, but I didn't find a good one for "one-side bound meaning that the field will be made available in the bounder"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

USES is indeed very bad.

@ncharles
Copy link
Member

ncharles commented Jul 5, 2016

shouldn't there be a test for the parsing of a metadata.xml that describes uses of new password type ?

@fanf
Copy link
Member Author

fanf commented Jul 6, 2016

PR rebased

@fanf fanf force-pushed the bug_8593/usermanagement_need_to_have_hashed_password_for_both_linux_and_aix branch from af4d6ac to be683ac Compare July 6, 2016 15:09
ncharles
ncharles reviewed Jul 6, 2016
View reviewed changes
src/test/scala/com/normation/cfclerk/domain/VariableTest.scala
v.spec.constraint.typeName.name == "masterPassword"
}

s"Have hash algorithme of type ${algos.map( _.prefix).mkString(",")}" in {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

algorithm

@fanf
Copy link
Member Author

fanf commented Jul 6, 2016

PR rebased

@fanf fanf force-pushed the bug_8593/usermanagement_need_to_have_hashed_password_for_both_linux_and_aix branch from be683ac to bedb90d Compare July 6, 2016 16:18
@fanf
Copy link
Member Author

fanf commented Jul 6, 2016

I propose to NOT merge it until we find a better word than "USES"

@fanf
Copy link
Member Author

fanf commented Jul 12, 2016

PR rebased

1 similar comment
@fanf
Copy link
Member Author

fanf commented Jul 13, 2016

PR rebased

@fanf fanf force-pushed the bug_8593/usermanagement_need_to_have_hashed_password_for_both_linux_and_aix branch from bedb90d to a62d958 Compare July 13, 2016 11:38
ncharles
ncharles reviewed Jul 13, 2016
View reviewed changes
src/main/scala/com/normation/cfclerk/domain/AixPasswordHashAlgo.scala
* Appart for md5, which is the standard unix implementation and differs only for the
* prefix ({smd5} in place of "$1", the other implementations differ SIGNIFICANTLY from
* standard Unix crypt described at https://www.akkadia.org/drepper/SHA-crypt.txt. In fact,
* they only kepted:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kept

@fanf fanf force-pushed the bug_8593/usermanagement_need_to_have_hashed_password_for_both_linux_and_aix branch from a62d958 to a70706a Compare July 13, 2016 17:17
@fanf
Copy link
Member Author

fanf commented Jul 13, 2016

PR rebased

1 similar comment
@fanf
Copy link
Member Author

fanf commented Jul 13, 2016

PR rebased

@fanf fanf force-pushed the bug_8593/usermanagement_need_to_have_hashed_password_for_both_linux_and_aix branch 2 times, most recently from 97ae51a to 16fab0a Compare July 18, 2016 12:22
@fanf
Copy link
Member Author

fanf commented Jul 18, 2016

PR rebased

ncharles
ncharles reviewed Jul 18, 2016
View reviewed changes
src/main/scala/com/normation/cfclerk/domain/AixPasswordHashAlgo.scala
private[this] final lazy val ssha256impl = getSecretKeFactory(ShaSpec.SHA256) match {
case Full(skf) => doSsha(ShaSpec.SHA256, skf) _
case e:Failure =>
// this may happen on Java and older version, because PBKDF2WithHmacSHA256
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess you meant java 7 and earlier ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, PBKDF2WithHmacSHA256 was introduced in Java 8. And PBKDF2WithHmacSHA1 in Java 6

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this comment makes no sense "this may happen on Java and older version"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, my brain was kindly dismissing the "and" part. Corrected

@fanf
Copy link
Member Author

fanf commented Jul 18, 2016

PR rebased

@fanf fanf force-pushed the bug_8593/usermanagement_need_to_have_hashed_password_for_both_linux_and_aix branch 2 times, most recently from 5b17a6c to f02c431 Compare July 18, 2016 16:51
@fanf
Copy link
Member Author

fanf commented Jul 18, 2016

PR rebased

1 similar comment
@fanf
Copy link
Member Author

fanf commented Jul 18, 2016

PR rebased

@fanf fanf force-pushed the bug_8593/usermanagement_need_to_have_hashed_password_for_both_linux_and_aix branch from f02c431 to 5b023fa Compare July 18, 2016 16:54
@fanf
Copy link
Member Author

fanf commented Jul 18, 2016

PR rebased

@fanf fanf force-pushed the bug_8593/usermanagement_need_to_have_hashed_password_for_both_linux_and_aix branch from 5b023fa to 75b39ab Compare July 18, 2016 16:56
@Normation-Quality-Assistant
Copy link

OK, merging this PR

@Normation-Quality-Assistant Normation-Quality-Assistant merged commit 75b39ab into Normation:branches/rudder/3.1 Jul 19, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
Ready for merge
Projects
None yet
Milestone
No milestone
4 participants
@fanf @ncharles @Normation-Quality-Assistant @matya