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

VaultTransitSecretEngine.readKey fails with asymmetric keys #16988

Closed
kdubb opened this issue May 5, 2021 · 6 comments · Fixed by #17024
Closed

VaultTransitSecretEngine.readKey fails with asymmetric keys #16988

kdubb opened this issue May 5, 2021 · 6 comments · Fixed by #17024
Labels
area/vault kind/bug Something isn't working
Milestone

Comments

@kdubb
Copy link
Contributor

kdubb commented May 5, 2021

Describe the bug

Invoking VaultTransitSecretEngine.readKey on a key with an asymmetric type (e.g. ecdsa-* or rsa-*) fails with a Jackson MismatchedInputException: Cannot deserialize value of type 'java.lang.Integer' from Object value (token 'JsonToken.START_OBJECT').

This is also a small feature request to provide the extended information that Vault returns for asymmetric keys.

Expected behavior

VaultTransitSecretEngine.readKey should properly handle asymmetric keys and provide the public key and its parameters (which Vault already includes in the output)

Actual behavior

VaultTransitSecretEngine.readKey assumes that the JSON response's keys property is a map of strings to integer timestamps. With asymmetric keys the keys property is a map of strings to maps of key info. Each key info map includes the public key (public_key), creation time (creation_time ), and rsa key size or ecdsa curve (name).

To Reproduce

Steps to reproduce the behavior:

  1. Create a transit key with an asymmetric type (e.g. ecdsa-p256), using VaultTransitSecretEngine.createKey is a valid method in addition to using the Vault cli or API.
  2. Attempt to read the key using VaultTransitSecretEngine.readKey

Screenshots

Here is a sample response from Vault's transit/keys/:key-id endpoint for an asymmetric key.

{
  "request_id": "c1d0fb48...",
  "lease_id": "",
  "renewable": false,
  "lease_duration": 0,
  "data": {
    "allow_plaintext_backup": false,
    "deletion_allowed": false,
    "derived": false,
    "exportable": false,
    "keys": {
      "1": {
        "creation_time": "2021-05-05T01:55:20.177039982Z",
        "name": "P-256",
        "public_key": "-----BEGIN PUBLIC KEY-----\n...\n-----END PUBLIC KEY-----\n"
      }
    },
    "latest_version": 1,
    "min_available_version": 0,
    "min_decryption_version": 1,
    "min_encryption_version": 0,
    "name": "test-796e638e",
    "supports_decryption": false,
    "supports_derivation": false,
    "supports_encryption": false,
    "supports_signing": true,
    "type": "ecdsa-p256"
  },
  "wrap_info": null,
  "warnings": null,
  "auth": null
}

Environment (please complete the following information):

Output of uname -a or ver

Darwin terminal.local 20.3.0 Darwin Kernel Version 20.3.0: Thu Jan 21 00:07:06 PST 2021; root:xnu-7195.81.3~1/RELEASE_X86_64 x86_64

Output of java -version

java version "11.0.6" 2020-01-14 LTS
Java(TM) SE Runtime Environment 18.9 (build 11.0.6+8-LTS)
Java HotSpot(TM) 64-Bit Server VM 18.9 (build 11.0.6+8-LTS, mixed mode)

Quarkus version or git rev

1.13.3.Final

Build tool (ie. output of mvnw --version or gradlew --version)

Additional context

(Add any other context about the problem here.)

@kdubb kdubb added the kind/bug Something isn't working label May 5, 2021
@quarkus-bot
Copy link

quarkus-bot bot commented May 5, 2021

/cc @vsevel

@sberyozkin
Copy link
Member

@vsevel Hi Vincent, would adding Map<String, VaultAsymmetricKey> to VaultTransitKeyDetail make sense ?

@vsevel
Copy link
Contributor

vsevel commented May 5, 2021

@sberyozkin I will be looking at it tonight

@kdubb
Copy link
Contributor Author

kdubb commented May 5, 2021

@sberyozkin @vsevel

To be clear in the JSON for symmetric keys, the keys map still contains integer values. So the Java map should probably be an interface (e.g. VaultKeyVersion) that has concrete subclasses for VaultSymmetricKeyVersion and VaultAsymmetricKeyVersion.

Because of the dynamic nature of the 'type' field (i.e. Vault can add new key types at will), I would imagine a custom deserializer is more appropriate than a scheme based on polymorphic decoding (e.g. @JsonSubTypes).

@sberyozkin
Copy link
Member

sberyozkin commented May 5, 2021

@kdubb That would require users casting and doing instanceof checks, Vincent will decide, but I guess as a user I'd prefer to work with typed asymmetric key beans; representing the symmetric ones as typed beans would be nice too, but it would be a breaking change - may be the method returning Map<String, Integer> can be deprecated...

kdubb added a commit to kdubb/quarkus that referenced this issue May 5, 2021
…etric keys

This fixes the current failure to read asymmetric keys while adding more information in a backwards compatible and futue proof format.

Fixes quarkusio#16988

### DTO Changes
* Changes `VaultTransitReadKeyData.keys` to be a list of `VaultTransitKeyVersionData`(which can contain any key data)
* Uses custom deserializer on `VaultTransitReadKeyData.keys` to allow `VaultTransitKeyVersionData` values to be deserialized from an integer timestamp or a map of info values.
* Added missing `latest_version`, ‘min_available_version` and `type` fields to `VaultTransitReadKeyData`

### API Changes
* Made `VaultTransitKeyDetail` abstract, subclasses for symmetric (`VaultTransitSymmetricKeyDetail`) and asymmetric (`VaultTransitAsymmetricKeyDetail`) keys are provided.
* Added abstract `versions` to `VaultTransitKeyDetail` which is a `Map<String, ? extends VaultTransitKeyDetail>`, subclasses of `VaultTransitKeyDetail` provide a concrete `versions` property with an unambiguous type.
* * E.g. `VaultTransitAsymmetricKeyDetail.versions` is defined as `Map<String, VaultTransitAsymmetricKey>`
* * Allows users to test the key detail type once (via `instanceof`) and downcast to a specifically typed list of version data.
* Added `latestVersion`, `minAvailableVersion` and `type` properties to `VaultTransitKeyDetail`
* Changes are additive and therefore backwards compatible
* Deprecates the `keys` property as `versions` provides the same information in a more easily accessible format and is future proof for adding more properties as Vault makes changes to the API.

### Tests
* Added integration test cases for reading ECDSA, RSA and AES keys

### Miscellaneous
* Added `JavaTimeModule` to the mapper used for vault responses.
kdubb added a commit to kdubb/quarkus that referenced this issue May 5, 2021
…etric keys

This fixes the current failure to read asymmetric keys while adding more information in a backwards compatible and futue proof format.

Fixes quarkusio#16988

### DTO Changes
* Changes `VaultTransitReadKeyData.keys` to be a list of `VaultTransitKeyVersionData`(which can contain any key data)
* Uses custom deserializer on `VaultTransitReadKeyData.keys` to allow `VaultTransitKeyVersionData` values to be deserialized from an integer timestamp or a map of info values.
* Added missing `latest_version`, `min_available_version` and `type` fields to `VaultTransitReadKeyData`

### API Changes
* Made `VaultTransitKeyDetail` abstract, subclasses for symmetric (`VaultTransitSymmetricKeyDetail`) and asymmetric (`VaultTransitAsymmetricKeyDetail`) keys are provided.
* Added abstract `versions` property to `VaultTransitKeyDetail` which is a `Map<String, ? extends VaultTransitKeyVersion>`, subclasses of `VaultTransitKeyDetail` provide a concrete `versions` property with an unambiguous type.
* * E.g. `VaultTransitAsymmetricKeyDetail.versions` is defined as `Map<String, VaultTransitAsymmetricKeyVersion>`
* * Allows users to test the key detail type once (via `instanceof`) and downcast to a detailt type that has a specifically typed list of versions.
* `VaultTransitKeyVersion` is abstract also, with subclasses for asymmetric (`VaultTransitAsymmetricKeyVersion`) and symmetric (`VaultTransitSymmetricKeyVersion`) keys
* Added `latestVersion`, `minAvailableVersion` and `type` properties to `VaultTransitKeyDetail`
* Changes are additive and therefore backwards compatible
* Deprecates the `keys` property as `versions` provides the same information in a more easily accessible format and is future proof for adding more properties as Vault makes changes to the API.

### Tests
* Added integration test cases for reading ECDSA, RSA and AES keys

### Miscellaneous
* Added `JavaTimeModule` to the mapper used for vault responses.
@kdubb
Copy link
Contributor Author

kdubb commented May 5, 2021

@sberyozkin PR #17024 fixes this issue, it adds a new versions and maintains the previous keys for backwards compatibility.

@vsevel It needs a reviewer but I'm not sure how to add one.

kdubb added a commit to kdubb/quarkus that referenced this issue May 7, 2021
…etric keys

This fixes the current failure to read asymmetric keys while adding more information in a backwards compatible and futue proof format.

Fixes quarkusio#16988

### DTO Changes
* Changes `VaultTransitReadKeyData.keys` to be a list of `VaultTransitKeyVersionData`(which can contain any key data)
* Uses custom deserializer on `VaultTransitReadKeyData.keys` to allow `VaultTransitKeyVersionData` values to be deserialized from an integer timestamp or a map of info values.
* Added missing `latest_version`, `min_available_version` and `type` fields to `VaultTransitReadKeyData`

### API Changes
* Made `VaultTransitKeyDetail` abstract, subclasses for symmetric (`VaultTransitSymmetricKeyDetail`) and asymmetric (`VaultTransitAsymmetricKeyDetail`) keys are provided.
* Added abstract `versions` property to `VaultTransitKeyDetail` which is a `Map<String, ? extends VaultTransitKeyVersion>`, subclasses of `VaultTransitKeyDetail` provide a concrete `versions` property with an unambiguous type.
* * E.g. `VaultTransitAsymmetricKeyDetail.versions` is defined as `Map<String, VaultTransitAsymmetricKeyVersion>`
* * Allows users to test the key detail type once (via `instanceof`) and downcast to a detailt type that has a specifically typed list of versions.
* `VaultTransitKeyVersion` is abstract also, with subclasses for asymmetric (`VaultTransitAsymmetricKeyVersion`) and symmetric (`VaultTransitSymmetricKeyVersion`) keys
* Added `latestVersion`, `minAvailableVersion` and `type` properties to `VaultTransitKeyDetail`
* Changes are additive and therefore backwards compatible
* Deprecates the `keys` property as `versions` provides the same information in a more easily accessible format and is future proof for adding more properties as Vault makes changes to the API.

### Tests
* Added integration test cases for reading ECDSA, RSA and AES keys

### Miscellaneous
* Added `JavaTimeModule` to the mapper used for vault responses.
kdubb added a commit to kdubb/quarkus that referenced this issue May 13, 2021
…etric keys

This fixes the current failure to read asymmetric keys while adding more information in a backwards compatible and futue proof format.

Fixes quarkusio#16988

* Changes `VaultTransitReadKeyData.keys` to be a list of `VaultTransitKeyVersionData`(which can contain any key data)
* Uses custom deserializer on `VaultTransitReadKeyData.keys` to allow `VaultTransitKeyVersionData` values to be deserialized from an integer timestamp or a map of info values.
* Added missing `latest_version`, `min_available_version` and `type` fields to `VaultTransitReadKeyData`

* Made `VaultTransitKeyDetail` abstract, subclasses for symmetric (`VaultTransitSymmetricKeyDetail`) and asymmetric (`VaultTransitAsymmetricKeyDetail`) keys are provided.
* Added abstract `versions` property to `VaultTransitKeyDetail` which is a `Map<String, ? extends VaultTransitKeyVersion>`, subclasses of `VaultTransitKeyDetail` provide a concrete `versions` property with an unambiguous type.
* * E.g. `VaultTransitAsymmetricKeyDetail.versions` is defined as `Map<String, VaultTransitAsymmetricKeyVersion>`
* * Allows users to test the key detail type once (via `instanceof`) and downcast to a detailt type that has a specifically typed list of versions.
* `VaultTransitKeyVersion` is abstract also, with subclasses for asymmetric (`VaultTransitAsymmetricKeyVersion`) and symmetric (`VaultTransitSymmetricKeyVersion`) keys
* Added `latestVersion`, `minAvailableVersion` and `type` properties to `VaultTransitKeyDetail`
* Changes are additive and therefore backwards compatible
* Deprecates the `keys` property as `versions` provides the same information in a more easily accessible format and is future proof for adding more properties as Vault makes changes to the API.

* Added integration test cases for reading ECDSA, RSA and AES keys

* Added `JavaTimeModule` to the mapper used for vault responses.
@quarkus-bot quarkus-bot bot added this to the 2.0 - main milestone May 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/vault kind/bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants