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

JsonWebKey from the Library is not compliant with IETF #16155

Closed
maxonweb opened this issue Oct 22, 2020 · 10 comments · Fixed by #24282
Closed

JsonWebKey from the Library is not compliant with IETF #16155

maxonweb opened this issue Oct 22, 2020 · 10 comments · Fixed by #24282
Assignees
Labels
Client This issue points to a problem in the data-plane of the library. customer-reported Issues that are reported by GitHub users external to the Azure organization. feature-request This issue requires a new behavior in the product in order be resolved. KeyVault needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team

Comments

@maxonweb
Copy link

Question:
Azure.Security.KeyVault.Keys.JsonWebKey returned from the Library as per https://docs.microsoft.com/en-us/dotnet/api/azure.security.keyvault.keys.keyvaultkey.key?view=azure-dotnet does not have kty, kid, key_ops properties for e.g.,

IETF Link: https://tools.ietf.org/html/rfc7517

@ghost ghost added needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. customer-reported Issues that are reported by GitHub users external to the Azure organization. question The issue doesn't require a change to the product in order to be resolved. Most issues start as that labels Oct 22, 2020
@jsquire jsquire added Client This issue points to a problem in the data-plane of the library. KeyVault needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team labels Oct 22, 2020
@ghost ghost removed the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Oct 22, 2020
@jsquire
Copy link
Member

jsquire commented Oct 22, 2020

Thank you for your feedback. Tagging and routing to the team member best able to assist.

@jsquire jsquire added needs-team-triage Workflow: This issue needs the team to triage. and removed needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team labels Oct 22, 2020
@jsquire
Copy link
Member

jsquire commented Oct 22, 2020

@heaths: I'm not sure if this is better routed to the service team. Would you be so kind as to triage and redirect if needed?

@heaths
Copy link
Member

heaths commented Oct 22, 2020

Those properties are serialized according to the spec, but have different names across our SDKs following idiomatic language conventions. For .NET:

  • kty -> JsonWebKey.KeyType
  • kid -> JsonWebKey.Id
  • key_ops -> JsonWebKey.KeyOps

Other key parameters like k remain the same, though are cased idiomatically for the language, e.g. in .NET K.

Have you found a case where properties are not serialized to or from the Key Vault service correctly?

@AlexGhiondea AlexGhiondea added needs-author-feedback Workflow: More information is needed from author to address the issue. and removed needs-team-triage Workflow: This issue needs the team to triage. labels Oct 22, 2020
@maxonweb
Copy link
Author

Which I can understand, but when I convert the Azure.Security.KeyVault.Keys.JsonWebKey object to Json, I expect the JSON produced to be IETF compliant. So with properties name KeyType, Id, KeyOps in the JSON it is not a IETF compliant JWK.

@ghost ghost added needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team and removed needs-author-feedback Workflow: More information is needed from author to address the issue. labels Oct 22, 2020
@heaths
Copy link
Member

heaths commented Oct 23, 2020

How are you converting it? Note we don't currently support serializing the JWK outside the SDK. When de/serialized within the SDK (like importing to or getting a key from Key Vault), then JWK is formatted correctly.

@heaths heaths added needs-author-feedback Workflow: More information is needed from author to address the issue. and removed needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team labels Oct 23, 2020
@maxonweb
Copy link
Author

If I am not able to get a IETF compliant JSON out of the JsonWebKey object I am not going call it as IETF compliant. The previous library Microsoft.Azure.KeyVault.WebKey ToString() method used to produce JSON which was IETF compliant. This new library Azure.Security does not exhibit the same behavior - https://docs.microsoft.com/en-gb/dotnet/api/microsoft.azure.keyvault.webkey.jsonwebkey.tostring?view=azure-dotnet-legacy#Microsoft_Azure_KeyVault_WebKey_JsonWebKey_ToString

I was able to do

new Microsoft.IdentityModel.Tokens.JsonWebKeySet(Microsoft.Azure.KeyVault.WebKey.ToString())

which does not work the same way in this new library, when I do

new Microsoft.IdentityModel.Tokens.JsonWebKeySet(Azure.Security.KeyVault.Keys.JsonWebKey.ToString())

Reference:
https://docs.microsoft.com/en-us/dotnet/api/microsoft.identitymodel.tokens.jsonwebkeyset.-ctor?view=azure-dotnet#Microsoft_IdentityModel_Tokens_JsonWebKeySet__ctor_System_String_

@ghost ghost added needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team and removed needs-author-feedback Workflow: More information is needed from author to address the issue. labels Oct 26, 2020
@heaths heaths added feature-request This issue requires a new behavior in the product in order be resolved. and removed question The issue doesn't require a change to the product in order to be resolved. Most issues start as that labels Oct 26, 2020
@heaths heaths added this to the [2020] December milestone Oct 26, 2020
@jsquire jsquire modified the milestones: [2020] December, Backlog Dec 7, 2020
@ghost ghost added the no-recent-activity There has been no recent activity on this issue. label Mar 12, 2021
@ghost
Copy link

ghost commented Mar 12, 2021

Hi @maxonweb. Thank you, for your interest in helping to improve the Azure SDK experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days.

@ghost ghost removed the no-recent-activity There has been no recent activity on this issue. label Mar 13, 2021
@ghost
Copy link

ghost commented Mar 13, 2021

Hi @maxonweb. There was a mistake and this issue was unintentionally flagged as a stale pull request. The label has been removed and the issue will remain active; no action is needed on your part. Apologies for the inconvenience.

@heaths
Copy link
Member

heaths commented Sep 16, 2021

@heaths
Copy link
Member

heaths commented Sep 27, 2021

Will add a JsonWebKeyConverter (since we standardize on System.Text.Json), but wanted to note that it will currently ignore any JsonSerializerOptions passed to it. Given it's an industry-standard format already, I suspect case-insensitive name handling unnecessary. Should it ever become necessary, it would not be hard to support by passing relevant properties like JsonSerializerOptions.PropertyNameCaseInsensitive or JsonSerializerOptions.PropertyNamingPolicy to the internal JsonWebKey.ReadProperties and JsonWebKey.WriteProperties methods.

heaths added a commit to heaths/azure-sdk-for-net that referenced this issue Sep 28, 2021
heaths added a commit that referenced this issue Sep 30, 2021
* Support serializing JWK using RFC 7517

Resolves #16155

* Make JsonWebKeyConverter internal
@github-actions github-actions bot locked and limited conversation to collaborators Mar 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Client This issue points to a problem in the data-plane of the library. customer-reported Issues that are reported by GitHub users external to the Azure organization. feature-request This issue requires a new behavior in the product in order be resolved. KeyVault needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants