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

[SIOP] Is request_obj_signing_alg really necessary? #23

Closed
dstrockis opened this issue Aug 26, 2019 · 19 comments · Fixed by #30
Closed

[SIOP] Is request_obj_signing_alg really necessary? #23

dstrockis opened this issue Aug 26, 2019 · 19 comments · Fixed by #30
Labels
did-authn-siop Issues related to the DID AuthN SIOP spec PR exists A PR exists for a specific issue

Comments

@dstrockis
Copy link

The current draft of the SIOP spec gives this example of a signed request:

{
   "alg": "ES256K",
   "typ": "JWT",
   "kid": "did:example:0xab#veri-key1"
}.
{
    "iss": "did:example:0xab",
    "response_type": "id_token",
    "client_id": "https://my.rp.com/cb",
    "scope": "openid did_authn",
    "state": "af0ifjsldkj",
    "nonce": "n-0S6_WzA2Mj",
    "response_mode" : "query",
    "registration" : {
        "request_object_signing_alg" : "ES256K",
        "jwks_uri" : "did:example:0xab",
        "id_token_signed_response_alg" : [ "ES256K", "Ed25519", "RS256" ],
    }
}

My question is, isn't the registration.request_object_signing_alg property repetitive here? Isn't the request object's signing algorithm already indicated in the alg property of the header? And for that matter, isn't registration.jwks_uri also unnecessary?

I am aware that the OpenID Connect core spec says the following in section 6.3.2:

To perform Signature Validation, the alg Header Parameter in the JOSE Header MUST match the value of the request_object_signing_alg set during Client Registration [OpenID.Registration] or a value that was pre-registered by other means. The signature MUST be validated against the appropriate key for that client_id and algorithm.

But, in a self-issued provider, where the registration details are included as part of the auth request, I'm not sure if the above check provides any value security-wise.

Curious if you had an explicit reason for requiring the request_object_signing_alg parameter, I may very well be missing something. Thanks!

@OR13
Copy link

OR13 commented Aug 26, 2019

Both are optional Client Metadata, however per the spec:

https://openid.net/specs/openid-connect-registration-1_0.html

OPTIONAL. JWS [JWS] alg algorithm [JWA] that MUST be used for signing Request Objects sent to the OP. All Request Objects from this Client MUST be rejected, if not signed with this algorithm. Request Objects are described in Section 6.1 of OpenID Connect Core 1.0 [OpenID.Core]. This algorithm MUST be used both when the Request Object is passed by value (using the request parameter) and when it is passed by reference (using the request_uri parameter). Servers SHOULD support RS256. The value none MAY be used. The default, if omitted, is that any algorithm supported by the OP and the RP MAY be used.

I agree, this is verbosity, not security, however, given the values are mostly DRAFT JWS, I think its wise to be explicit here...

Regarding your second point,

For the sake on interoperability, I think we should not limit SIOP to DID only, and for that reason I would argue the jwks_uri is not redundant.

It seems reasonable for an rfc7638 kid and normal jwks uri to be used, in this case, you would need both properties because the kid would not be sufficient to locate the signing key.

An example of this kind of configuration would be 2 desktop applications that use OIDC SIOP and an internet tunnel (like ngrok) to allow authentication.

In this case, both would not necessarily need DID at all, but would want to rely on openid:// and https://trader1.exchange.example.com/.well-known/jwks.json...

@dstrockis
Copy link
Author

Ok, I see your point about jwks_uri, no arguments there.

I understand the purpose of purpose of the request_object_signing_alg when used in dynamic client registration for classic OpenID Connect flows. But I'm still a little confused about its purpose in a self-issued flow.

How is a self-issued OP supposed to "reject, if not signed with this algorithm"? The client has not been previously registered with the self-issued OP, so how could the OP know which algorithm(s) to accept/reject?

The self-issued section of the spec says:

The Registration parameters that would typically be used in requests to Self-Issued OPs are policy_uri, tos_uri, and logo_uri. If the Client uses more than one Redirection URI, the redirect_uris parameter would be used to register them. Finally, if the Client is requesting encrypted responses, it would typically use the jwks_uri, id_token_encrypted_response_alg and id_token_encrypted_response_enc parameters.

This makes me think that request_object_signing_alg is not really meant to be used in self-issued flows... It's not serving any purpose here other than communicating the algorithm used in the current request - which is already provided in the header.

We're currently working on an implementation of this SIOP spec, and I'd like to omit the request_object_signing_alg from our implementation unless there's a real reason for its inclusion.

@OR13
Copy link

OR13 commented Aug 29, 2019

yes, I see your point and agree with it. I think the only reason for including it would be verbosity, and per the quote above:

The default, if omitted, is that any algorithm supported by the OP and the RP MAY be used.

its probably safe to exclude given this statement.

@awoie awoie added the did-authn-siop Issues related to the DID AuthN SIOP spec label Nov 5, 2019
@awoie
Copy link
Member

awoie commented Nov 5, 2019

I agree that request_object_signing_alg can be omitted. I will provide a PR for that.

With respect to jwks_uri. If we want to a JWKS reconstituted from a DID Document, then we would have to assume that this will be a DID URL and the respective resolver will have a feature to return a JWKS.
@OR13 @dstrockis Would you agree with that?

awoie pushed a commit that referenced this issue Nov 5, 2019
@awoie awoie added the PR exists A PR exists for a specific issue label Nov 5, 2019
@awoie
Copy link
Member

awoie commented Nov 5, 2019

@dstrockis @OR13 I reviewed the current spec text and the OIDC spec again, I don't see the need for jwks and jwks_uri being mandatory anymore. Of course, a DID AuthN-enabled SIOP-OP will have to understand both as they are part of the OIDC spec, but a A DID AuthN-enabled SIOP-RP would not include them to share their keys. I would assume that they use their DID or DID Document instead. Further, the goal of the spec is to stay backward compatible with existing OIDC clients rather than SIOP-OPs. Thoughts?

@OR13
Copy link

OR13 commented Nov 5, 2019

I'd rather SIOP not include any DID specific types (DID Doc), and instead have it only include OIDC types, such as jwks, or jwks_url.

I see the goal of the spec as to maximize the legacy support for DIDs, so the assumption should be that OP/RP should be able to use SIOP without any understanding of DID types.

@awoie
Copy link
Member

awoie commented Nov 5, 2019

Okay, then jwks will be a plain JWKS that matches a subset of the RP's DID Doc. So, jwks and jwks_uri will stay mandatory for signed request objects or when an encrypted response id_token is expected. There is no other way to tell plain SIOP-OPs which key should be used to verify the signature, or encrypt the response id_token.

@OR13 my other question was how a DID URL pointing to a JWKS inside a DID Doc might look like. Some resolvers might support dereferencing a DID URL to a JWKS directly, others might not. Do you think we should allow DID URLs as jwks_uri values? This could break existing plain SIOP-OPs that are not able to dereference DID URLs, on the other hand it would dramatically reduce the size of the request object, i.e., QR Code (if being used).

@dstrockis
Copy link
Author

Sounds good to me. I don't have an opinion on the DID URL question, I leave it to you guys.

@OR13
Copy link

OR13 commented Nov 6, 2019

I'm not a fan of using DID URLS in this flow, but I could see jwks_uri being of 2 forms:

  1. did:example:123456789abcdefghi;service=jwks_transform?query=...#fra

  2. https://example.com/resolver/did:example:123456789abcdefghi/jwks.json

I'd prefer we start with 2 since 1 can be created on top of it, and 2 assumes http, and does not require the concept of a resolver.

@peacekeeper
Copy link
Member

I feel the right approach would be to first define what a DID URL would look like, e.g.:

did:example:123456789abcdefghi;transform-keys=jwks

And then the standard HTTP Binding for DID resolution could be used to construct an HTTP URL that can simply be retrieved via GET, e.g.:

https://uniresolver.io/1.0/identifiers/did:example:123456789abcdefghi;transform-keys=jwks

This way you could achieve two goals: 1. The ability to have a working HTTP URL for software that doesn't know how to resolve DIDs, and 2. Also allow software that supports it to do native, local DID resolution with all the benefits that come from that (not having to rely on a third-party DID resolver service).

If you think this makes sense, I could try to hack this into the UR as an experimental feature?

@OR13
Copy link

OR13 commented Nov 6, 2019

^ much better. I still prefer the use of http uri first.

@awoie
Copy link
Member

awoie commented Nov 7, 2019

I think that is a good example of where a HTTP(S)-based DID resolver makes sense. This allows great interoperability with traditional HTTP-based URLs:

  1. DID-enabled SIOP-RPs MUST include jwks_uri (for interop with plain SIOP-OPs): https://uniresolver.io/1.0/identifiers/did:example:123456789abcdefghi;transform-keys=jwks
  2. Plain SIOP-RPs MAY include jwks or jwks_uri as follows:
    https://my.siop-rp.com/jwks.json

Edited:

  1. DID-enabled SIOP-OPs would check whether did_authn scope is present and jwks_uri is used:
  • if did_authn and local resolver is present, use the local resolver or another remote resolver to get the JWKS but MUST ensure that iss matches the DID in the URL. If local resolver fails to resolve the JWKS (e.g., DID method not supported), continue ...
  • if did_authn and no local resolver is present or failed to resolve, just use the remote resolver URL but MUST ensure that iss matches the DID in the URL.
  1. Plain SIOP-OPs would ignore the did_authn scope and follow the SIOP protocol. This MAY include obtaining the remote resolver JWKS using a normal HTTP client

I will provide some info in the paper how this should be used.

@awoie
Copy link
Member

awoie commented Nov 7, 2019

@OR13 @peacekeeper I removed my last comment because I believe the security concern that I had could be mitigated by mandating that DID-enabled SIOP-RPs have to always include a remote resolver jwks_uri for interoperability with plain SIOP-OPs. Do you think that is acceptable? A DID-enabled SIOP-OP could always ignore the remote resolver and use their own local or remote resolver.

@awoie awoie closed this as completed in #30 Nov 11, 2019
awoie pushed a commit that referenced this issue Nov 11, 2019
fixes #23: removed mandatory `request_object_signing_alg` from `registration` parameter

After our last call I double checked that there are no limitations on the values of iss and kid in request objects. I'm going to merge this PR and close the issue. If there is still an issue we should open another more specific issue in the papers repo.
@peacekeeper
Copy link
Member

Based on this thread, I added an experimental feature to the Universal Resolver that can transform public keys to JWK (Set), see here: https://hackmd.io/XmL-Bjh5TdqV4fj6nwdPEQ

@OR13
Copy link

OR13 commented Nov 21, 2019

@peacekeeper this is excellent! we have some code that we use client side to do this to did documents returned by the universal resolver, this is much better, and its nice to be able to use matrix params for something this useful.

Note that the crv "P-256K" is legacy / deprecated, https://github.com/panva/jose#secp256k1

https://tools.ietf.org/html/draft-ietf-cose-webauthn-algorithms-01

Changed the JOSE curve identifier from "P-256K" to "secp256k1".

@ChristopherA
Copy link

Be aware that signatures in secp256k1 for ECDSA are different than signatures for secp256k1 for Schnorr. As there are some real advantages to Schnorr over ECDSA (provably secure, batching, aggregation, privacy, etc.) Bitcoin and other systems are transitioning to Schnorr.

@OR13
Copy link

OR13 commented Nov 22, 2019

@ChristopherA for JOSE, it seems like we need a draft for IETF for Schnorr... https://tools.ietf.org/html/draft-ietf-cose-webauthn-algorithms-01

We should circle up with @selfissued and get a plan in place for that.

@ChristopherA
Copy link

ChristopherA commented Nov 22, 2019

The spec for secp256k1 x-only Schnorr-based pubkeys and signatures (aka BIP-Schnorr) is at https://github.com/sipa/bips/blob/bip-schnorr/bip-schnorr.mediawiki, the test vectors at https://github.com/sipa/bips/blob/bip-schnorr/bip-schnorr/test-vectors.csv, the python reference (but not constant time) implementation is at https://github.com/sipa/bips/blob/bip-schnorr/bip-schnorr/reference.py, and a C-based constant-time implementation based on the security reviewed secp256k1 (but the Schnorr additions have not yet been formally security reviewed) is at https://github.com/BlockchainCommons/secp256k1-schnorrsig. A security justification for x-only pubkeys is at https://medium.com/blockstream/reducing-bitcoin-transaction-sizes-with-x-only-pubkeys-f86476af05d7.

These specs and reference implementations are considered mature, with original proposal from July 2018 and lots of review by the Bitcoin community, with the x-only pubkeys proposal being the only major change since. The authors do not expect any changes and this spec is being considered for an upcoming soft-fork to Bitcoin called "taproot", which has over 180 reviewers of the spec and code (including this spec.)

-- Christopher Allen

@OR13
Copy link

OR13 commented Nov 22, 2019

There is also https://github.com/bitauth/bitcoin-ts

which we have used for browser support for secp256k1 before.

Seems like it would be helpful to the bitcoin community for someone to propose a JOSE draft for BIP-Schnorr support that added a JWK format for x-only Schnorr-based pubkeys, and JWS for Schnorr signatures.

This could then be used to in a subsequent JSON-LD Signature suite.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
did-authn-siop Issues related to the DID AuthN SIOP spec PR exists A PR exists for a specific issue
Projects
None yet
5 participants