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

#93 Add support for asymmetric keys to vault transit engine #100

Merged
merged 2 commits into from
Sep 6, 2024

Conversation

MattDavis00
Copy link
Contributor

As described in #93 the current implementation fails to deserialize a read transit engine key response if the key is asymmetric. The asymmetric response differs as it includes the public key inside of the keys field:

// vault@926ee8c21172:/$ curl -XGET --insecure --silent -H "X-Vault-Token: $VAULT_TOKEN" $VAULT_ADDR/v1/transit/keys/my-new-key
{
    "request_id": "b59d5612-b16f-8742-641c-32e29943433f",
    "lease_id": "",
    "renewable": false,
    "lease_duration": 0,
    "data": {
        "allow_plaintext_backup": false,
        "auto_rotate_period": 0,
        "deletion_allowed": false,
        "derived": false,
        "exportable": false,
        "imported_key": false,
        "keys": {
            "1": {
                "creation_time": "2024-05-10T13:36:49.809157592Z",
                "name": "rsa-2048",
                "public_key": "-----BEGIN PUBLIC KEY-----\nMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEA01QgwZl/tD7cWZl1NPQ2\nG8+cvI6KkF+wB94xfatdKJ07Ga0bsqYaeE98ZUZU7amRH9Kmdz/4Cu9xXdwlosSS\ntedXfRT8zcWpqEUpV4c4/lT/ox6W67KW2l44liiVt2f9PmviRoYclNWUJa6X+ZCU\n68p/Rd9RSb/xBsywTb8uJN1Q1cZM2JbKhWnaLa7jPsXG9hvnxJ2QFuN3+iZ2OCff\nCjhl0Anumc7lL6wEU/Yd1WuX+5afcHaGttHkWAasrhq3KFHhXlwf4+jrJ5C+aOfb\nevJgGUYjXgf4buV3sPFVmnhvyiyyEV/CysNbXx3KrI/OGgf7HQ+f3+hnpomo8lhd\nwwIDAQAB\n-----END PUBLIC KEY-----\n"
            },
            "2": {
                "creation_time": "2024-05-10T13:36:49.878041842Z",
                "name": "rsa-2048",
                "public_key": "-----BEGIN PUBLIC KEY-----\nMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEA7Ttq3kD0Tev7uWnupVGS\neGl4IURKqiUpoPKdevpnYTfuKt/MVP9Ej1Qz7XOP3q6mC3Vb0mSK+6FNjWNakd7D\nQecJwVFuZXp4lF8uzTF5xXFiPiiw5WMS7v7oNmzTE15Msse1yLHcxQGveeRmrl1L\nTYzcLHb705AzgeXoHqLNNWfTC72fqlPhMvOIcWyWOatz/Dp9ukfcR7HAmxhlpluY\n8e+lift8xah4uQFFDuIwXBQ7f5G8+j9RKMyWjA8pv/pY12jDl/vOi6V2b5YJvJie\nVV9gNa1+9JjamG1bsmHYhRIK0rBTewdvD3Tyg+T8Yl5HQNqkIvdwaBZ/RIc+PVeI\ndwIDAQAB\n-----END PUBLIC KEY-----\n"
            },
            "3": {
                "creation_time": "2024-05-10T13:36:49.958724342Z",
                "name": "rsa-2048",
                "public_key": "-----BEGIN PUBLIC KEY-----\nMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAu5FQYHEs8U1QsnkTMDrB\n6MfpGK1eVdS84FepYyvpA//suDbJGS9l6XP333wuebwoTL42B5qiZ1tudbIeOWaK\nBDQ5dfjbjRB87k/h4nuQ2DNEUwhgVPjxS3XEocefVWxCfmwQqTjSvhVX2l3DmWiE\n1b/vgikBcYxkqTDTUytuN+IbvZmkFzcM+20UaemRQ5VeNP2MEjZsX4cBATFMkC6k\nYoXb3r+SeKd5JDp/qCSUeabkaxBoV1MprXCy8sEgxSEn/1SGOlVQvB9COQQ/Y/bq\nHQUNGKgrQWGXh4W+Uz4C/GIsKosVuTcGV/o/eONg5uft958NTa1vf93XaKpLOlbw\nOQIDAQAB\n-----END PUBLIC KEY-----\n"
            }
        },
        "latest_version": 3,
        "min_available_version": 0,
        "min_decryption_version": 1,
        "min_encryption_version": 0,
        "name": "my-new-key",
        "supports_decryption": true,
        "supports_derivation": false,
        "supports_encryption": true,
        "supports_signing": true,
        "type": "rsa-2048"
    },
    "wrap_info": null,
    "warnings": null,
    "auth": null
}

My implementation follows the normal serde deserialization process using enums to define the two types of key responses. As these fields are pub, this will likely be a breaking change for anybody relying on the ReadKeyResponse.keys value.

@MattDavis00 MattDavis00 changed the title #93 Add support for asymmetric to vault transit engine #93 Add support for asymmetric keys to vault transit engine Jun 25, 2024
Copy link
Collaborator

@Haennetz Haennetz left a comment

Choose a reason for hiding this comment

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

The other API endpoints that also use the creation_time attribute use the type String. We shouldn't mix the type of the attributes.

Cargo.toml Outdated Show resolved Hide resolved
src/api/transit/responses.rs Outdated Show resolved Hide resolved
src/api/transit/responses.rs Outdated Show resolved Hide resolved
@Haennetz
Copy link
Collaborator

It would be nice if you could add a test in the file https://github.com/jmgilman/vaultrs/blob/master/tests/transit.rs

@MattDavis00
Copy link
Contributor Author

The other API endpoints that also use the creation_time attribute use the type String. We shouldn't mix the type of the attributes.

I can swap that to a String; ideally the dates crate-wide would be dates in my opinion.

I might look into whether adding a chrono feature flag to the crate is feasible; that swaps all of the String dates to chrono::DateTime. I haven't had much time to check the current uses.

@MattDavis00
Copy link
Contributor Author

It would be nice if you could add a test in the file https://github.com/jmgilman/vaultrs/blob/master/tests/transit.rs

Will do. Thanks

@MattDavis00
Copy link
Contributor Author

I'm a bit limited on time at the moment; but I will make sure to get round to this at some point

…route

Previously the route assumed a symmetric key's creation unix timestamp would be returned.
For asymmetric keys the response differs; it returns the creation RFC3339 timestamp, public key, and key type.

Mentions jmgilman#93.

Signed-off-by: Matt Davis <mattdavis@cloudflare.com>
Signed-off-by: Matt Davis <mattdavis@cloudflare.com>
@MattDavis00
Copy link
Contributor Author

Updated to use String for the creation_time field, added some tests, and rebased

Copy link
Collaborator

@Haennetz Haennetz left a comment

Choose a reason for hiding this comment

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

Thanks for the work only one small question.

Comment on lines 40 to +41
base64 = "0.21"
chrono = "0.4.38"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we don't use this anymore right?

Suggested change
base64 = "0.21"
chrono = "0.4.38"
base64 = "0.21"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is used as a dev-dependency to check the creation_time in the tests can be parsed. This differs from the symmetric route, as vault returns a unix integer timestamp for that; whereas for asymmetric keys it returns an ISO8601 timestamp. TLDR; checks that vault returns ISO8601

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MattDavis00
Copy link
Contributor Author

Thanks @Haennetz. Any chance on getting this merged?

@Haennetz Haennetz merged commit 0d4c17d into jmgilman:master Sep 6, 2024
8 checks passed
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.

2 participants