New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ELY-2320] Add integrity support to FileSystemSecurityRealm #1709
Conversation
@fjuma @Skyllarr @OndrejKotek |
78476ad
to
930e194
Compare
7fb7e33
to
5bb9d35
Compare
auth/realm/base/pom.xml
Outdated
@@ -85,6 +85,11 @@ | |||
<groupId>org.wildfly.common</groupId> | |||
<artifactId>wildfly-common</artifactId> | |||
</dependency> | |||
<dependency> | |||
<groupId>org.apache.santuario</groupId> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since there was some discussion about this one, it would be good to add a comment here to explain the reason that this dependency is being used.
IllegalArgumentException invalidKeyPairArgument(String s); | ||
|
||
@Message(id = 13011, value = "Unable to access master index file: %s") | ||
IllegalStateException unableToAccessMainIndex(String s); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this used by anything now?
|
||
@LogMessage(level = Logger.Level.TRACE) | ||
@Message(id = 13012, value = "Unable to write checksum to main index") | ||
void unableToWriteToMainIndex(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
*/ | ||
public FileSystemSecurityRealm(final Path root, final NameRewriter nameRewriter, final int levels, final boolean encoded, final Encoding hashEncoding, final Charset hashCharset, final Supplier<Provider[]> providers, final SecretKey secretKey) { | ||
public FileSystemSecurityRealm(final Path root, final NameRewriter nameRewriter, final int levels, final boolean encoded, final Encoding hashEncoding, final Charset hashCharset, Supplier<Provider[]> providers, final SecretKey secretKey, final PrivateKey privateKey, final PublicKey publicKey) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason to remove final
from the providers
parameter?
@@ -252,10 +317,10 @@ public FileSystemSecurityRealm(final Path root, final NameRewriter nameRewriter, | |||
* @param nameRewriter the name rewriter to apply to looked up names | |||
* @param levels the number of levels of directory hashing to apply | |||
* @param hashEncoding the string format for hashed passwords. Uses Base64 by default. | |||
* @param hashCharset the character set to use when converting password strings to a byte array. Uses UTF-8 by default and must not be {@code null}. | |||
* @param hashCharset the character set to use when converting password strings to a byte array. Uses UTF-8 by default and must not be {@code null}. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor but looks like extra whitespace was introduced here.
@@ -0,0 +1,71 @@ | |||
/* | |||
* JBoss, Home of Professional Open Source. | |||
* Copyright 2020 Red Hat, Inc., and individual contributors |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2022 :)
@@ -0,0 +1,150 @@ | |||
<?xml version="1.0" encoding="UTF-8"?> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When adding a new schema file, it's good to first copy/paste the existing file and just update the version and include this in a commit. Then introduce the new element(s) in a follow-up commit. This makes it easier to see the specific changes introduced. We also tend to create a separate ELY issue to track adding the new client schema version.
<xsd:all minOccurs="1" maxOccurs="1"> | ||
<xsd:element name="principal" type="name-type" minOccurs="1" maxOccurs="1"/> | ||
<xsd:element name="credentials" type="credentials-type" minOccurs="0" maxOccurs="1"/> | ||
<xsd:element name="attributes" type="attributes-type" minOccurs="0" maxOccurs="1"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The signature element should be added here too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's an example here that shows how to import and make use of an element from another schema:
@@ -26,7 +26,6 @@ | |||
import java.lang.reflect.Constructor; | |||
import java.security.NoSuchAlgorithmException; | |||
import java.security.Provider; | |||
import java.security.Provider.Service; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this being removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It wasn't in use, although it's unrelated to my PR, I can revert this change
<credentials> | ||
<password algorithm="clear" format="base64">AXNlY3JldFBhc3N3b3Jk</password> | ||
</credentials> | ||
<Signature xmlns="http://www.w3.org/2000/09/xmldsig#"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just thinking about the formatting issue we just discussed. Is this formatting valid here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It isn't but this tests is checking an invalid signature. I thought it would be good for someone to be able to visualise the signature tag a bit better.
Should I set it to what the realm normally generates instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the signature only invalid due to the formatting or due to the actual content too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The content is also invalid, seen here
wildfly-elytron/tests/base/src/test/resources/filesystem-realm-exists/u/s/e/user-OVZWK4Q.xml
Line 18 in c4f7abc
<SignatureValue>INVALID_SIGNATURE</SignatureValue> |
117ae93
to
6a52254
Compare
@fjuma I've addressed all comments |
23cfe36
to
7e09a5d
Compare
.setLevels(1) | ||
.setProviders(ELYTRON_PASSWORD_PROVIDERS); | ||
if (mode.equals("encryption") || mode.equals("both")) { securityRealmBuilder.setSecretKey(secretKey); } | ||
else if (mode.equals("integrity") || mode.equals("both")) { securityRealmBuilder.setPrivateKey(privateKey); securityRealmBuilder.setPublicKey(publicKey); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Ashpan Just a minor, the else should be removed here. Now if mode.equals("both")
is true, the private and public key are not set
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah good catch
while (realmIterator.hasNext()) { | ||
Identity identity = (Identity) realmIterator.next(); | ||
if(! identity.verifyIntegrity()) { | ||
identity.dispose(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that's a good point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Theres a trace log here that runs whenever we verify any identity, should I still add another one here?
Line 1500 in 7e09a5d
ElytronMessages.log.tracef("FileSystemSecurityRealm - Error during verification. Signature for credential [%s] failed", name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Ashpan Ah yes, sorry I missed that. Although it could still happen that the validatePrincipalName(path.toString(), name)
method fails instead of the digital signature method, and then no identity is logged right? So we can add a log to the validatePrincipalName method also and we know the identity is logged then
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah thats true, I'll add another log in validatePrincipalName()
@Test | ||
public void testCreateIdentityWithLevelsEncryption() throws Exception { | ||
@Test | ||
public void testCreateIdentityWithLevelsIntegrity() throws Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to check because it's a bit hard to tell from the diff, was the encryption test removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't look like the tests validate the signature that's written to the identity file. That would be good to add.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the encryption test got moved around, Ive put them all together now
newIdentity.dispose(); | ||
|
||
ModifiableRealmIdentity existingIdentity = securityRealm.getRealmIdentityForUpdate(new NamePrincipal("p")); | ||
securityRealm = FileSystemSecurityRealm.builder() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no need to recreate the security realm, the existing instance can be used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
.setPrivateKey(privateKey) | ||
.setPublicKey(publicKey) | ||
.setProviders(ELYTRON_PASSWORD_PROVIDERS) | ||
.build(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a general comment. Since the same builders are being used in multiple places, might be better to introduce methods that return the two common builders.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
.setRoot(getRootPath()) | ||
.setLevels(3) | ||
.setProviders(ELYTRON_PASSWORD_PROVIDERS); | ||
if (mode.equals("encryption")) { securityRealmBuilder.setSecretKey(secretKey); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of this approach, it would be better to add secretKey
, privateKey
, and publicKey
parameters to createAndLoadAndDeleteIdentity
and then just set the builder methods if the values that are passed in aren't null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
KeyPair pair = keyPairGen.generateKeyPair(); | ||
PrivateKey newPrivateKey = pair.getPrivate(); | ||
PublicKey newPublicKey = pair.getPublic(); | ||
securityRealm = FileSystemSecurityRealm.builder() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to recreate the security realm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
newIdentity.setCredentials(Collections.singleton(new PasswordCredential(credential))); | ||
newIdentity.dispose(); | ||
|
||
securityRealm = FileSystemSecurityRealm.builder() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
@@ -1089,4 +1348,22 @@ private Path getRootPath() throws Exception { | |||
return getRootPath(true); | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a general comment. Have you tested the case where the identity file has been tampered with?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you tested the case where the signature can't be written to the file for some reason and then verified that the file remains unchanged?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a general comment. Have you tested the case where the identity file has been tampered with?
Added 2 tests where identity is tampered.
Have you tested the case where the signature can't be written to the file for some reason and then verified that the file remains unchanged?
I've tested manually, I'm unable to make a test to get the code into a state where only during writing the signature does it fail. If I try to make that happen, it gets caught in the "verify signature" method instead of the unable to write signature portion, but I am now using the temporary file to make changes before copying that to the final file.
@Ashpan Please also link ELY-2345 in the description here (that makes it easier for us to resolve the issues once approved). Thanks. |
<xsd:element name="credentials" type="credentials-type" minOccurs="0" maxOccurs="1"/> | ||
<xsd:element name="attributes" type="attributes-type" minOccurs="0" maxOccurs="1"/> | ||
<xsd:element name="Signature" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A type
attribute should be added here that references the SignatureType
from the xml dsig schema.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should set minOccurs
and maxOccurs
too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
* Re-generate the signatures for all the identities in this realm. | ||
* This method is intended to be called after updating the key pair used by this realm. | ||
* | ||
* @throws RealmUnavailableException Thrown if unable to obtain the realm identity iterator or dispose and close the realm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could reword this to something like:
@throws RealmUnavailableException if the realm is not able to handle requests for any reason
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
<xsd:element name="credentials" type="credentials-type" minOccurs="0" maxOccurs="1"/> | ||
<xsd:element name="attributes" type="attributes-type" minOccurs="0" maxOccurs="1"/> | ||
<xsd:element name="Signature" type="SignatureType" minOccurs="1" maxOccurs="1"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this needs to be type="xmldsig:SignatureType"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't minOccurs be 0 since it's possible to have unencrypted files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this needs to be
type="xmldsig:SignatureType"
Updated
Shouldn't minOccurs be 0 since it's possible to have unencrypted files?
Ah good catch, updated
|
||
<xsd:complexType name="identity-type"> | ||
<xsd:all minOccurs="1" maxOccurs="1"> | ||
<xsd:element name="principal" type="name-type" minOccurs="1" maxOccurs="1"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this element is only added if integrity is enabled, minOccurs should be 0 here too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
// Mixed versions unsupported. | ||
throw ElytronMessages.log.fileSystemRealmInvalidContent(path, streamReader.getLocation().getLineNumber(), name); | ||
} | ||
|
||
if (version.isAtLeast(Version.VERSION_1_2) && "principal".equals(streamReader.getLocalName())) { | ||
consumeContent(streamReader); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to be a bit careful here. We need to make sure that if the principal element is encountered and the version isn't at least 1.2, then the invalid content (i.e., fileSystemRealmInvalidContent
) should be thrown.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added error if version mismatch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Just to simplify the logic and make it a bit easier to understand what's happening, I think you could update it to:
if ("principal".equals(streamReader.getLocalName()) {
if (version.isAtLeast(...)) {
...
} else {
...
}
}
893200a
to
0650738
Compare
|
||
public static class IntegrityResult { | ||
private final boolean valid; | ||
private final ArrayList<Identity> identities; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wondering if this could be an ArrayList<String>
instead? That way, you'd just build up the list while verifying integrity and wouldn't need to iterate over the identities in getIdentityNames
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was just thinking if in the future this was to be used for another purpose and another property of the identity was required, it would be useful. Although I could change it to ArrayList<String> identityNames
for this use case, and if the identities are required in the future, the identities arraylist could be added back
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think we just need the string names for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
@@ -1358,4 +1654,26 @@ public XMLStreamWriter getXmlStreamWriter() { | |||
return xmlStreamWriter; | |||
} | |||
} | |||
|
|||
public static class IntegrityResult { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Javadoc should be added for this class and its methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added
http://issues.redhat.com/browse/ELY-2345
https://issues.redhat.com/browse/ELY-2320