PasswordSpec encoders/decoders and filesystem-realm echancements. #246

Merged
merged 2 commits into from Jul 23, 2015

Conversation

Projects
None yet
4 participants
@pedroigor
Contributor

pedroigor commented Jul 21, 2015

This pull request contains two commits:

  • 6d7f443 is all about supporting encoders and decoders for PasswordSpec types. It also contains enhancements to the ``PasswordUtil` in order to better separate MCF and non-MCF helper methods.
  • b7f8b01 is all about changes to to the FileSystemSecurityRealm in order to better support credential management and some minor fixes.

This PR depends on #248.

pom.xml
@@ -55,7 +55,7 @@
<version.hsqldb>2.3.1</version.hsqldb>
<version.org.wildfly.checkstyle-config>1.0.3.Final</version.org.wildfly.checkstyle-config>
<version.org.wildfly.client.config>1.0.0.Alpha2</version.org.wildfly.client.config>
- <version.org.wildfly.common>1.0.0.Alpha3</version.org.wildfly.common>
+ <version.org.wildfly.common>1.0.0.Alpha4-SNAPSHOT</version.org.wildfly.common>

This comment has been minimized.

@pedroigor

pedroigor Jul 21, 2015

Contributor

Depends on wildfly/wildfly-common@60b631d.

Build is failing because of that.

@pedroigor

pedroigor Jul 21, 2015

Contributor

Depends on wildfly/wildfly-common@60b631d.

Build is failing because of that.

}
public void create() throws RealmUnavailableException {
for (;;) {
final Path tempPath = tempPath();
final XMLOutputFactory xmlOutputFactory = XMLOutputFactory.newFactory();
- try (OutputStream outputStream = new BufferedOutputStream(Files.newOutputStream(tempPath, WRITE, CREATE_NEW, DSYNC))) {
+ try (OutputStream outputStream = Files.newOutputStream(tempPath, CREATE_NEW, WRITE, DSYNC)) {

This comment has been minimized.

@dmlloyd

dmlloyd Jul 22, 2015

Contributor

Why?

@dmlloyd

dmlloyd Jul 22, 2015

Contributor

Why?

This comment has been minimized.

@pedroigor

pedroigor Jul 22, 2015

Contributor

No specific reason, going to revert that change..

@pedroigor

pedroigor Jul 22, 2015

Contributor

No specific reason, going to revert that change..

import org.wildfly.security.password.interfaces.ClearPassword;
+import org.wildfly.security.password.spec.ClearPasswordSpec;
+import org.wildfly.security.password.spec.DefaultPasswordSpecEncoding;
+import org.wildfly.security.password.spec.PasswordSpec;
import org.wildfly.security.util.ByteIterator;
import org.wildfly.security.util.CodePointIterator;

This comment has been minimized.

@dmlloyd

dmlloyd Jul 22, 2015

Contributor

Why refactor this whole class? Only a minor change is needed AFAICT. The refactor makes it hard to see the functional difference.

@dmlloyd

dmlloyd Jul 22, 2015

Contributor

Why refactor this whole class? Only a minor change is needed AFAICT. The refactor makes it hard to see the functional difference.

This comment has been minimized.

@pedroigor

pedroigor Jul 22, 2015

Contributor

The main change here is about how to write the identity. So we could reuse how the top and bottom level elements are created and customize only the actual body.

This is the case where you are creating or updating the identity.

I can also revert that, if you want.

@pedroigor

pedroigor Jul 22, 2015

Contributor

The main change here is about how to write the identity. So we could reuse how the top and bottom level elements are created and customize only the actual body.

This is the case where you are creating or updating the identity.

I can also revert that, if you want.

This comment has been minimized.

@pedroigor

pedroigor Jul 22, 2015

Contributor

Also, there were some issues when parsing the xml. So I fixed them.

@pedroigor

pedroigor Jul 22, 2015

Contributor

Also, there were some issues when parsing the xml. So I fixed them.

This comment has been minimized.

@dmlloyd

dmlloyd Jul 22, 2015

Contributor

Given that this code is not really reviewable as-is, it would be ideal to have separate commits for each change.

@dmlloyd

dmlloyd Jul 22, 2015

Contributor

Given that this code is not really reviewable as-is, it would be ideal to have separate commits for each change.

This comment has been minimized.

@pedroigor

pedroigor Jul 22, 2015

Contributor

Ok, let me apply the changes to the original file. I'll try to keep them more clear.

@pedroigor

pedroigor Jul 22, 2015

Contributor

Ok, let me apply the changes to the original file. I'll try to keep them more clear.

+ byte[] hash1 = keySpec.getHash();
+ ByteBuffer specBytes = ByteBuffer.allocate(1 + 4 + SALT_SIZE + hash1.length)
+ .put(ITERATED_SALTED_HASH_SPEC_ID) // 1 byte identifying the password spec
+ .putInt(keySpec.getIterationCount()); // 4 bytes representing the iteration count

This comment has been minimized.

@dmlloyd

dmlloyd Jul 22, 2015

Contributor

This is another good case for packed integers.

@dmlloyd

dmlloyd Jul 22, 2015

Contributor

This is another good case for packed integers.

This comment has been minimized.

@dmlloyd

dmlloyd Jul 22, 2015

Contributor

In general I think ByteStringBuilder would have been a better choice for this, since this is why we have it.

@dmlloyd

dmlloyd Jul 22, 2015

Contributor

In general I think ByteStringBuilder would have been a better choice for this, since this is why we have it.

This comment has been minimized.

@pedroigor

pedroigor Jul 22, 2015

Contributor

Hummm ... I can try it out. Didn't notice it.

Regarding packed integers, should we go to this path then ? And pack salt and iteration count in a single integer ?

@pedroigor

pedroigor Jul 22, 2015

Contributor

Hummm ... I can try it out. Didn't notice it.

Regarding packed integers, should we go to this path then ? And pack salt and iteration count in a single integer ?

+ }
+
+ private static PasswordSpec decodeIteratedSaltedHashPasswordSpec(ByteBuffer buffer) {
+ // decode the iteration count

This comment has been minimized.

@dmlloyd

dmlloyd Jul 22, 2015

Contributor

ByteIterator could have been better here.

@dmlloyd

dmlloyd Jul 22, 2015

Contributor

ByteIterator could have been better here.

+ byte[] hash = keySpec.getHash();
+ ByteBuffer specBytes = ByteBuffer.allocate(1 + SALT_SIZE + hash.length)
+ .put(SALTED_HASH_PASSWORD_SPEC_ID); // 1 byte identifying the password spec
+ // SALT_SIZE bytes representing the salt, 0s are appended to the initial bytes to fill unused bytes.

This comment has been minimized.

@dmlloyd

dmlloyd Jul 22, 2015

Contributor

Using fixed-length fields does provide a solution here, but two other options exist: pack the entire field as a packed int (wastes 1 bit per byte), or provide a packed-int length followed by raw bytes (more efficient for longer fields).

Here's the breakdown of spatial efficiency:

Size Length Field Packed
0 1 0
1 2 2
2 3 3
3 4 4
4 5 5
5 6 6
6 7 7
7 8 8
8 9 10
9 10 11
10 11 12
11 12 13
12 13 14
13 14 15
14 15 16
15 16 18
16 17 19
17 18 20
18 19 21
19 20 22
20 21 23
21 22 24
22 23 26
23 24 27
24 25 28
25 26 29
26 27 30
27 28 31

So packed is better when the field is up to 7 bytes long, after which a packed length field + raw bytes is better.

@dmlloyd

dmlloyd Jul 22, 2015

Contributor

Using fixed-length fields does provide a solution here, but two other options exist: pack the entire field as a packed int (wastes 1 bit per byte), or provide a packed-int length followed by raw bytes (more efficient for longer fields).

Here's the breakdown of spatial efficiency:

Size Length Field Packed
0 1 0
1 2 2
2 3 3
3 4 4
4 5 5
5 6 6
6 7 7
7 8 8
8 9 10
9 10 11
10 11 12
11 12 13
12 13 14
13 14 15
14 15 16
15 16 18
16 17 19
17 18 20
18 19 21
19 20 22
20 21 23
21 22 24
22 23 26
23 24 27
24 25 28
25 26 29
26 27 30
27 28 31

So packed is better when the field is up to 7 bytes long, after which a packed length field + raw bytes is better.

This comment has been minimized.

@dmlloyd

dmlloyd Jul 22, 2015

Contributor

If the field cannot be empty, then length + raw bytes is always better.

@dmlloyd

dmlloyd Jul 22, 2015

Contributor

If the field cannot be empty, then length + raw bytes is always better.

@pedroigor

This comment has been minimized.

Show comment
Hide comment
@pedroigor

pedroigor Jul 22, 2015

Contributor

I think we can add the encode(PasswordSpec) as well keep the encode(Password). The latter makes life easier for client code.

Contributor

pedroigor commented Jul 22, 2015

I think we can add the encode(PasswordSpec) as well keep the encode(Password). The latter makes life easier for client code.

+ * @param password the password to encode
+ * @return a byte array representing the encoded password or null if no encoder was capable to encode the given password
+ */
+ public static byte[] encode(Password password) throws NoSuchAlgorithmException, InvalidKeySpecException {

This comment has been minimized.

@dmlloyd

dmlloyd Jul 22, 2015

Contributor

If this method is retained, then I suggest that instead of doing isInstance checks (which BTW are better written using instanceof), you can do:

if (passwordFactory.convertibleToKeySpec(password, ClearPasswordSpec.class)) {
    return encodeClearPasswordSpec(passwordFactory.getKeySpec(ClearPasswordSpec.class));
} ...

This way you can support as-yet-unknown types.

@dmlloyd

dmlloyd Jul 22, 2015

Contributor

If this method is retained, then I suggest that instead of doing isInstance checks (which BTW are better written using instanceof), you can do:

if (passwordFactory.convertibleToKeySpec(password, ClearPasswordSpec.class)) {
    return encodeClearPasswordSpec(passwordFactory.getKeySpec(ClearPasswordSpec.class));
} ...

This way you can support as-yet-unknown types.

@@ -70,13 +59,7 @@ private PasswordUtil() {}
private static final int A_APACHE_HTDIGEST = 7;
private static final int A_BSD_CRYPT_DES = 8;
private static final int A_CRYPT_DES = 9;
- private static final int A_DIGEST_MD2 = 10;

This comment has been minimized.

@dmlloyd

dmlloyd Jul 22, 2015

Contributor

As we discussed yesterday, we could continue to support custom types in the modular crypt string. But if we do so, I think we should move to a more standard $id$content syntax instead of the ad-hoc mixture at present. The only exceptions are the old crypt password types which are already recognizable in a standard way.

@dmlloyd

dmlloyd Jul 22, 2015

Contributor

As we discussed yesterday, we could continue to support custom types in the modular crypt string. But if we do so, I think we should move to a more standard $id$content syntax instead of the ad-hoc mixture at present. The only exceptions are the old crypt password types which are already recognizable in a standard way.

This comment has been minimized.

@pedroigor

pedroigor Jul 22, 2015

Contributor

Why we need to support them here ?

@pedroigor

pedroigor Jul 22, 2015

Contributor

Why we need to support them here ?

This comment has been minimized.

@dmlloyd

dmlloyd Jul 22, 2015

Contributor

We don't need to, but it might be useful to modular crypt applications like supporting Apache-style .htdigest files or UNIX /etc/password formatted files.

@dmlloyd

dmlloyd Jul 22, 2015

Contributor

We don't need to, but it might be useful to modular crypt applications like supporting Apache-style .htdigest files or UNIX /etc/password formatted files.

This comment has been minimized.

@pedroigor

pedroigor Jul 22, 2015

Contributor

That means we need to provide encoding/decoding to all those types we support in BasicPasswordSpecEncoding in ModularCrypt ?

If so, I've already done it. Just need to grab from my history ...

@pedroigor

pedroigor Jul 22, 2015

Contributor

That means we need to provide encoding/decoding to all those types we support in BasicPasswordSpecEncoding in ModularCrypt ?

If so, I've already done it. Just need to grab from my history ...

This comment has been minimized.

@dmlloyd

dmlloyd Jul 22, 2015

Contributor

Yes, that would be necessary, along with standardizing the $id$xxxx format stuff.

@dmlloyd

dmlloyd Jul 22, 2015

Contributor

Yes, that would be necessary, along with standardizing the $id$xxxx format stuff.

This comment has been minimized.

@dmlloyd

dmlloyd Jul 22, 2015

Contributor

We should probably prefix all of our algorithms with ely- to be safe.

@dmlloyd

dmlloyd Jul 22, 2015

Contributor

We should probably prefix all of our algorithms with ely- to be safe.

This comment has been minimized.

@pedroigor

pedroigor Jul 22, 2015

Contributor

That is what I did in #238.

Is that it ?

@pedroigor

pedroigor Jul 22, 2015

Contributor

That is what I did in #238.

Is that it ?

This comment has been minimized.

@pedroigor

pedroigor Jul 22, 2015

Contributor

But now we use the spec id instead of the algorithm as specified on that PR.

@pedroigor

pedroigor Jul 22, 2015

Contributor

But now we use the spec id instead of the algorithm as specified on that PR.

This comment has been minimized.

@pedroigor

pedroigor Jul 22, 2015

Contributor

Actually, I need we still need to use the algo.

@pedroigor

pedroigor Jul 22, 2015

Contributor

Actually, I need we still need to use the algo.

This comment has been minimized.

@dmlloyd

dmlloyd Jul 22, 2015

Contributor

We could actually make it completely generic. If the password type isn't recognized as a modular crypt type, then we pass it to the password spec encoder, which then uses convertibleToKeySpec to determine if the password can be encoded. Then new passwords automatically get modular crypt support too (albeit not under a standard name).

@dmlloyd

dmlloyd Jul 22, 2015

Contributor

We could actually make it completely generic. If the password type isn't recognized as a modular crypt type, then we pass it to the password spec encoder, which then uses convertibleToKeySpec to determine if the password can be encoded. Then new passwords automatically get modular crypt support too (albeit not under a standard name).

+ * @return a byte array representing the encoded password or null if no encoder was capable to encode the given password
+ */
+ public static byte[] encode(PasswordSpec passwordSpec) throws NoSuchAlgorithmException, InvalidKeySpecException {
+ if (ClearPasswordSpec.class.isInstance(passwordSpec)) {

This comment has been minimized.

@dmlloyd

dmlloyd Jul 22, 2015

Contributor

These should use instanceof checks.

@dmlloyd

dmlloyd Jul 22, 2015

Contributor

These should use instanceof checks.

+ public static byte[] encode(Password password) throws NoSuchAlgorithmException, InvalidKeySpecException {
+ PasswordFactory passwordFactory = PasswordFactory.getInstance(password.getAlgorithm());
+
+ if (ClearPassword.class.isInstance(password)) {

This comment has been minimized.

@dmlloyd

dmlloyd Jul 22, 2015

Contributor

See my note on the previous diff here: the passwordFactory should be queried using the convertibleToKeySpec method instead of instanceof checks.

@dmlloyd

dmlloyd Jul 22, 2015

Contributor

See my note on the previous diff here: the passwordFactory should be queried using the convertibleToKeySpec method instead of instanceof checks.

- ClearPassword clearPassword = (ClearPassword) credential;
+ ClearPassword clearPassword = null;
+ // we only know how to verify plain-text passwords
+ if (char[].class.isInstance(credential)) {

This comment has been minimized.

@dmlloyd

dmlloyd Jul 22, 2015

Contributor

credential instanceof char[]

@dmlloyd

dmlloyd Jul 22, 2015

Contributor

credential instanceof char[]

streamWriter.writeStartDocument();
streamWriter.writeCharacters("\n");
- streamWriter.writeStartElement(ELYTRON_1_0, "identity");
+ streamWriter.writeStartElement("", "identity", ELYTRON_1_0);

This comment has been minimized.

@dmlloyd

dmlloyd Jul 22, 2015

Contributor

Why this change?

@dmlloyd

dmlloyd Jul 22, 2015

Contributor

Why this change?

This comment has been minimized.

@pedroigor

pedroigor Jul 22, 2015

Contributor

We need a prefix, in this case an empty one.

@pedroigor

pedroigor Jul 22, 2015

Contributor

We need a prefix, in this case an empty one.

+
+ if (encoded != null) {
+ format = BASE64_FORMAT;
+ ByteIterator.ofBytes(encoded).base64Encode().drainTo(passwordString);

This comment has been minimized.

@dmlloyd

dmlloyd Jul 22, 2015

Contributor

You could skip the StringBuilder here and go right to a String.

@dmlloyd

dmlloyd Jul 22, 2015

Contributor

You could skip the StringBuilder here and go right to a String.

@dmlloyd dmlloyd added the +1 DML label Jul 22, 2015

@dmlloyd

This comment has been minimized.

Show comment
Hide comment
@dmlloyd

dmlloyd Jul 22, 2015

Contributor

Looks good except for the failed tests.

Once the tests are resolved and #248 is merged, this will need a rebase, then it will be ready AFAICT.

Contributor

dmlloyd commented Jul 22, 2015

Looks good except for the failed tests.

Once the tests are resolved and #248 is merged, this will need a rebase, then it will be ready AFAICT.

@wildfly-ci

This comment has been minimized.

Show comment
Hide comment
@wildfly-ci

wildfly-ci Jul 23, 2015

Linux Build 653 is now running using a merge of c23aa7e

Linux Build 653 is now running using a merge of c23aa7e

@wildfly-ci

This comment has been minimized.

Show comment
Hide comment
@wildfly-ci

wildfly-ci Jul 23, 2015

Windows Build 659 is now running using a merge of c23aa7e

Windows Build 659 is now running using a merge of c23aa7e

@wildfly-ci

This comment has been minimized.

Show comment
Hide comment
@wildfly-ci

wildfly-ci Jul 23, 2015

Linux Build 653 outcome was SUCCESS using a merge of c23aa7e
Summary: Tests passed: 570, ignored: 3 Build time: 0:01:53

Linux Build 653 outcome was SUCCESS using a merge of c23aa7e
Summary: Tests passed: 570, ignored: 3 Build time: 0:01:53

@wildfly-ci

This comment has been minimized.

Show comment
Hide comment
@wildfly-ci

wildfly-ci Jul 23, 2015

Windows Build 659 outcome was SUCCESS using a merge of c23aa7e
Summary: Tests passed: 570, ignored: 3 Build time: 0:02:19

Windows Build 659 outcome was SUCCESS using a merge of c23aa7e
Summary: Tests passed: 570, ignored: 3 Build time: 0:02:19

@wildfly-ci

This comment has been minimized.

Show comment
Hide comment
@wildfly-ci

wildfly-ci Jul 23, 2015

Windows Build 661 is now running using a merge of 262ea13

Windows Build 661 is now running using a merge of 262ea13

@wildfly-ci

This comment has been minimized.

Show comment
Hide comment
@wildfly-ci

wildfly-ci Jul 23, 2015

Linux Build 655 is now running using a merge of 262ea13

Linux Build 655 is now running using a merge of 262ea13

@wildfly-ci

This comment has been minimized.

Show comment
Hide comment
@wildfly-ci

wildfly-ci Jul 23, 2015

Linux Build 655 outcome was SUCCESS using a merge of 262ea13
Summary: Tests passed: 578, ignored: 3 Build time: 0:01:52

Linux Build 655 outcome was SUCCESS using a merge of 262ea13
Summary: Tests passed: 578, ignored: 3 Build time: 0:01:52

@wildfly-ci

This comment has been minimized.

Show comment
Hide comment
@wildfly-ci

wildfly-ci Jul 23, 2015

Windows Build 661 outcome was SUCCESS using a merge of 262ea13
Summary: Tests passed: 578, ignored: 3 Build time: 0:02:34

Windows Build 661 outcome was SUCCESS using a merge of 262ea13
Summary: Tests passed: 578, ignored: 3 Build time: 0:02:34

@pedroigor

This comment has been minimized.

Show comment
Hide comment
@pedroigor

pedroigor Jul 23, 2015

Contributor

Rebased.

Contributor

pedroigor commented Jul 23, 2015

Rebased.

@pedroigor pedroigor removed the rebase this label Jul 23, 2015

@sguilhen sguilhen added the +1 SG label Jul 23, 2015

sguilhen added a commit that referenced this pull request Jul 23, 2015

Merge pull request #246 from pedroigor/ELY-223
PasswordSpec encoders/decoders and filesystem-realm echancements.

@sguilhen sguilhen merged commit 68df2da into wildfly-security:master Jul 23, 2015

1 check passed

default TeamCity Build WildFly projects :: Elytron :: PR aggregator finished: Running
Details

@pedroigor pedroigor deleted the pedroigor:ELY-223 branch Nov 22, 2016

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