Skip to content

Key Vault: Resolve inconsistency between spec and service/client implementation #12966

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.

Sign up for GitHub

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

Merged
lmazuel merged 1 commit into from
Feb 17, 2021
Merged

Key Vault: Resolve inconsistency between spec and service/client implementation #12966

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 3 additions & 6 deletions specification/keyvault/data-plane/Microsoft.KeyVault/preview/7.2-preview/securitydomain.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@
}
}
},
"/securitydomain/transferkey": {
"/securitydomain/upload": {
"get": {
"tags": [
"HSMSecurityDomain"
Expand Down Expand Up @@ -148,9 +148,7 @@
"$ref": "./examples/securitydomaintransferkey-example.json"
}
}
}
},
"/securitydomain/upload": {
},
"post": {
"tags": [
"HSMSecurityDomain"
Expand Down Expand Up @@ -291,8 +289,7 @@
"properties": {
"value": {
"type": "string",
"format": "base64url",
Copy link
Member

@heaths heaths Feb 12, 2021

Choose a reason for hiding this comment

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

A format is still required. This blob must be encoded somehow. Even if a valid UTF-8 string now (e.g. JavaScript or Python source, as @herveyw-msft pondered as a thought experiment), this format might change in the future. The encoding rules would need to stay the same for SDKs to continue working properly.

Thus, the required format (which is most often base64url in KV) must be specified in swagger.

Copy link
Member

@heaths heaths Feb 12, 2021

Choose a reason for hiding this comment

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

As we discussed offline, I'm concerned this isn't future-proof. The fact this blob is opaque and could be anything is exactly why it shouldn't be simply string-encoded, which also requires that it's valid UTF-8 or can be transcoded correctly and automatically by the language SDK.

For a "v1" (specifically, 7.2) you could always assume it's UTF-8 string-encoded JSON, but I do recommend that in 7.3-preview you add a contentType or contentEncoding property (like the HTTP headers) like we did for Secure Key Release (now in 7.3-preview) that provides a "hint" to both the client and service as to what it really is. Absence of this continues to imply UTF-8 string-encoded JSON, but presence could indicate whatever the property says - be it encoded binary data or even just "application/json".

To note, the format here only dictates the encoding over the wire. It does not and should not affect how the data is stored by the client or service. So, for example, security domains that are JSON files today would still be JSON files even if you were to leave this "base64url". The only thing that changes it that the CLI and service would base64url-encode or -decode it before hand.

/cc @herveyw-msft @vickm

"description": "A blob containing the Security Domain."
"description": "The Security Domain."
}
},
"description": "The Security Domain.",
Expand Down
15 changes: 6 additions & 9 deletions specification/keyvault/data-plane/Microsoft.KeyVault/preview/7.3-preview/securitydomain.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@
"final-state-via": "azure-async-operation"
},
"operationId": "HSMSecurityDomain_Download",
"description": "Retrieves the Security Domain from the managed HSM. Calling this endpoint can be used to activated a provisioned managed HSM resource.",
"description": "Retrieves the Security Domain from the managed HSM. Calling this endpoint can be used to activate a provisioned managed HSM resource.",
"parameters": [
{
"in": "body",
Expand All @@ -74,7 +74,7 @@
"schema": {
"$ref": "#/definitions/CertificateInfoObject"
},
"description": "Security Domain download operation requires customer to provide N certificates (minimum 3 and maximum 10) containing public key in JWK format."
"description": "The Security Domain download operation requires customer to provide N certificates (minimum 3 and maximum 10) containing a public key in JWK format."
},
{
"$ref": "#/parameters/ApiVersionParameter"
Expand Down Expand Up @@ -117,7 +117,7 @@
}
}
},
"/securitydomain/transferkey": {
"/securitydomain/upload": {
"get": {
"tags": [
"HSMSecurityDomain"
Expand Down Expand Up @@ -148,9 +148,7 @@
"$ref": "./examples/securitydomaintransferkey-example.json"
}
}
}
},
"/securitydomain/upload": {
},
"post": {
"tags": [
"HSMSecurityDomain"
Expand Down Expand Up @@ -291,8 +289,7 @@
"properties": {
"value": {
"type": "string",
"format": "base64url",
"description": "A blob containing the Security Domain."
"description": "The Security Domain."
}
},
"description": "The Security Domain.",
Expand Down Expand Up @@ -328,7 +325,7 @@
},
"kty": {
"type": "string",
"description": "JsonWebKey Key Type (kty), as defined in https://tools.ietf.org/html/draft-ietf-jose-json-web-algorithms-40. For Security Domain this value must be RSA"
"description": "JsonWebKey Key Type (kty), as defined in https://tools.ietf.org/html/draft-ietf-jose-json-web-algorithms-40. For Security Domain this value must be RSA."
},
"key_ops": {
"type": "array",
Expand Down