Skip to content
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 #462

Merged
merged 1 commit into from
Nov 9, 2022

Conversation

Ashpan
Copy link
Contributor

@Ashpan Ashpan commented Mar 18, 2022

```bash
/subsystem=elytron/filesystem-realm=fsRealm:add(path=fs-realm-users, relative-to=jboss.server.config.dir, key-store=keystore, key-store-alias=localhost)
```
The signature will be added to the identity xml file at the end before the closing `</identity>` tag.

Choose a reason for hiding this comment

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

Is an XML standard applied?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

----
RUxZAUMQUO9rq4PRc3BB61B5oZSepATonjzjUriqbsy//5fw:RUxZAUMQ41IKUmpazIiXdB5oZSep4wQa67I+y35TLtssweZ
----
==== Hard Requirements

Choose a reason for hiding this comment

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

Is it possible to enable this feature for an existing realm without a need of conversion? In case the encryption feature the elytron-tool has to be used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes this is doable, will add to Elytron and then update this document

Choose a reason for hiding this comment

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

The process should be the opposite way - plan first, then implementation. It seems we don't need elytron-tool for this functionality as there is the update-key-pair CLI operation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry will update this plan first.
And that is correct we don't need the elytron tool feature, the CLI operation will work for this.


== Overview

This objective of this proposal is to add the ability to verify integrity for the filesystem realm, this will be done with the use of a Public and Private Key pair, that will create a signature on every identity on the xml identity file.

Choose a reason for hiding this comment

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

This objective of this proposal is to add the ability to verify integrity for the filesystem realm

The whole filesystem realm, or just individual identities? I'm able to add a new identity with name of my choice just by copying an existing one. It is also possible to override an existing identity this way.


== Overview

This objective of this proposal is to add the ability to verify integrity for the filesystem realm, this will be done with the use of a Public and Private Key pair, that will create a signature on every identity on the xml identity file.

Choose a reason for hiding this comment

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

This objective of this proposal is to add the ability to verify integrity for the filesystem realm

The whole filesystem realm, or just individual identities? I'm able to add a new identity with name of my choice just by copying an existing one. It is also possible to override an existing identity this way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've changed it so now you could copy and existing identity but any other change to the identity (eg changing attributes, name, credentials) will flag as an error next time the identity is being referenced

@Ashpan Ashpan force-pushed the integrity-for-filesystem-realms branch from e9a835f to f8211e6 Compare May 4, 2022 19:51
@Ashpan Ashpan force-pushed the integrity-for-filesystem-realms branch from f8211e6 to b0b4556 Compare June 3, 2022 14:19

== Overview

This objective of this proposal is to add the ability to verify integrity for the filesystem realm, this will be done with the use of a Public and Private Key pair, that will create a signature on every identity on the xml identity file.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/that will create a signature on every identity on the xml identity file/that will be used to add a signature for each identity in the xml identity files.


To store the KeyPair, a key store will be created and the FileSystem realm will utilize the key store to access the PublicKey and PrivateKey to verify filesystem integrity.

For the default out of box configuration for WildFly, the goal is for the FileSystemSecurityRealm to have integrity enabled.
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be good to mention that this would enabled as part of a separate RFE.

== Requirements
=== Integrity of Filesystem Realm

Filesystem Integrity is a planned upcoming change, but will not be added until after WildFly 26.1 is released
Copy link
Contributor

Choose a reason for hiding this comment

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

This line can be removed now.


It could be made that the public key is available for everyone to use. This means anyone can decrypt the signature to get the hash of the identity, but only people with access to the key store for the realm would have access to the private key that encrypts the hash of each identity. This way an unauthorized user could view the hash via the public key, but cannot change the hash/signature of the identity.

The approach that is taken is to use the key pair file signature approach. To do so, ever time data is to be written or fetched by an identity, first the filesystem realm will verify the signature with identity file. If this signature ever does not match the identity, then the filesystem realm integrity has been compromised.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/by an identity/for an identity

Copy link
Contributor

Choose a reason for hiding this comment

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

s/If this signature ever does not match the identity/If signature verification fails

The identity file with integrity enabled via a Private and Public Key that contains a clear text password along with an attribute would look like the following.
```XML
<?xml version="1.0" encoding="UTF-8" standalone="no"?><identity xmlns="urn:elytron:1.0">
<credentials>
Copy link
Contributor

Choose a reason for hiding this comment

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

This example should be updated to include the new principal tag.

</credentials>
<attributes>
<attribute name="Roles" value="JBossAdmin"/>
</attributes><Signature xmlns="http://www.w3.org/2000/09/xmldsig#"><SignedInfo><CanonicalizationMethod Algorithm="http://www.w3.org/TR/2001/REC-xml-c14n-20010315"/><SignatureMethod Algorithm="http://www.w3.org/2001/04/xmldsig-more#rsa-sha256"/><Reference URI=""><Transforms><Transform Algorithm="http://www.w3.org/2000/09/xmldsig#enveloped-signature"/></Transforms><DigestMethod Algorithm="http://www.w3.org/2001/04/xmlenc#sha256"/><DigestValue>KzLR5/cf78iV3iT1L7fgl7z2Wq1XTvoD2FTqk9FXWYs=</DigestValue></Reference></SignedInfo><SignatureValue>IsOfSpixRMUmgdkuCy/gANiETAfEF5/XWIl8yKT50JjpH3ChGE5i/O1mO4YLPC4OwzilJPNSGHTb&#13;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the signature element here copy/pasted from an identity file? Is it possible to add proper formatting of this XML to the file?

== Implementation Plan
=== Filesystem Integrity

Filesystem Integrity is a planned upcoming change, but will not be added until after WildFly 26.1 is released
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this line can be removed.


It could be made that the public key is available for everyone to use. This means anyone can decrypt the signature to get the hash of the identity, but only people with access to the key store for the realm would have access to the private key that encrypts the hash of each identity. This way an unauthorized user could view the hash via the public key, but cannot change the hash/signature of the identity.

The approach that is taken is to use the key pair file signature approach. To do so, ever time data is to be written or fetched by an identity, first the filesystem realm will verify the signature with identity file. If this signature ever does not match the identity, then the filesystem realm integrity has been compromised.
Copy link
Contributor

@Skyllarr Skyllarr Jun 8, 2022

Choose a reason for hiding this comment

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

@Ashpan Just checking, does the key pair file signature approach mean computing the checksum of the file and signing this checksum?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry no, by that I meant signing the identity file with the use of a keypair
eg:

<?xml version="1.0" encoding="UTF-8" standalone="no"?><identity xmlns="urn:elytron:identity:1.2">
    <principal name="user"/>
    <credentials>
        <password algorithm="clear" format="base64">AXNlY3JldFBhc3N3b3Jk</password>
    </credentials>
    <Signature xmlns="http://www.w3.org/2000/09/xmldsig#"><SignedInfo><CanonicalizationMethod Algorithm="http://www.w3.org/TR/2001/REC-xml-c14n-20010315"/><SignatureMethod Algorithm="http://www.w3.org/2001/04/xmldsig-more#rsa-sha256"/><Reference URI=""><Transforms><Transform Algorithm="http://www.w3.org/2000/09/xmldsig#enveloped-signature"/></Transforms><DigestMethod Algorithm="http://www.w3.org/2001/04/xmlenc#sha256"/><DigestValue>xrU4pnFXL7ln3prQzFmmsFciNwRO/cE6TefYad4riho=</DigestValue></Reference></SignedInfo><SignatureValue>8FYmjywTC1uKFPxffL2GxXTU1e5r+ds2WcQ1BDcodiNq/tC</SignatureValue><KeyInfo><KeyValue><RSAKeyValue><Modulus>nQWmAHovgGbBjCdk/ihZ7JKr3M+YvXP6H700GQ834vmw8jBogB/D9l7DU5Tv5qcJngTo0haqKpzk&#13;
0YIGRApiD2Jcw7D/FgOM+R8FYmjywTC1uKFPxffL2GxXTU1e5r+ds2WcQ1BDcodiNq/tCclQyaXW&#13;
ME9o4Z0IPHgRP8N8uC+OkUuuhk0IIhDQyrLhQhDzR8MvDY1vcg9kCsg9t/ELd7VMVcV+QmDXUs+Z&#13;
xbdaKnaso2xUw8aHEniy7t6ontc7FMYWVaeF2vptB3tw9bLexynhqE1x53hDl9nDzkd8ah49s+JW&#13;
U6drnaYKM0ACXaLbkHjpMvqy1c4DRdqmP8apZw==</Modulus><Exponent>AQAB</Exponent></RSAKeyValue></KeyValue></KeyInfo></Signature></identity>

Copy link
Contributor

@Skyllarr Skyllarr Jun 8, 2022

Choose a reason for hiding this comment

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

@Ashpan Thank you. Tthe solution mentioned last time was to compute a checksum, and sign only that checksum. Which reduces the amount of data that is being processed by the signature algorithm. Should this be added to the analysis doc or was it decided to not use a checksum at all? Sorry if I am missing something

Copy link
Contributor Author

@Ashpan Ashpan Jun 8, 2022

Choose a reason for hiding this comment

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

@Skyllarr Oh I see the confusion. For the current time being we decided to not use the checksum at all anymore, and as a follow up task for this RFE, I'll create an issue for using the checksum to increase efficiency.

@Ashpan Ashpan force-pushed the integrity-for-filesystem-realms branch from b0b4556 to f59e387 Compare June 8, 2022 17:16
@bstansberry bstansberry merged commit 7affa0f into wildfly:main Nov 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants