WebId-OIDC Support

[rodant]

Introduction

The purpose of this PR is to implement support for the WebId-OIDC standard used by the Solid project. This would make possible an integration between Trellis and the Node Solid Server (NSS) like: A User accesses a resource in his POD hosted on Trellis (Resource Server and Relying Party) and uses his authentication data presented by a Solid client (Presenter) after a login step against a NSS server (Identity Provider).

I'm not an specialist in OAuth and OIDC protocols, please correct me if I miss something here.
The solid-auth-client library sends a so called Proof of Possession (PoP) token in the Authorization header as Bearer token.
This PoP token is constructed from my point of view according to the WebId-OIDC Spec.
In short the NSS server issues a JWT token on user login, which is embedded in a client generated token as id_token claim.
The id_token contains additionally public key information from the client for a signature validation aligning to the JWK specification. The following is an example of a PoP token extracted from requests an example Solid App sends:

{"alg":"RS256"}.{ "iss":"7bc774a71455546534c821686f2a5993", "aud":"https://orisha2.solid.community", "exp":1583334972, "iat":1583331372, "id_token":<embedded-id-token>, "token_type":"pop"}.<signature>

The embedded id_token is:
{"alg":"RS256", "kid":"m6hdkJtyAJM"}. { "iss": "https://solid.community", "sub": "https://orisha2.solid.community/profile/card#me", "aud": "7bc774a71455546534c821686f2a5993", "exp": 1574592332, "iat": 1573382732, "jti": "347676826d25d3d4", "nonce": "Ww9_k9r7cpiuL495V1X7Pnt5TvRsMJ0oQbxK5vGv-gs", "azp": "7bc774a71455546534c821686f2a5993", "cnf": { "jwk": { "alg": "RS256", "e": "AQAB", "ext": true, "key_ops": [ "verify" ], "kty": "RSA", "n": "jocytHgM-3L7P3PvUlIMXsaP9TAkQwun7HXd1QZ9eCcPiWXcu070hQx0ogNEg1uMArBGwzc8K4PJe0N8i4A2fpvLK8uHVOnSTaVXN8dPSRXlsFtLgRqBXxHsGXdjdRqKyasB4dxeYMKhajiJjr7P5YkBSyGL_BoKBBX5xC7cfZqmwbKYJ2kiKNF5EOp-NH6V9QlYWT2Yf6347s3Z5ltUPGUG30cxb3zu4gd9mV-kSO3jHhQ0u02n5yhQVrUe8jeOEGt_srPystoGZDW3gN_Isezf1YLpj7TEmLn8pbBRyNpi8iqV5lYmd2HAisCijRE5X2veTpbqgRXL1HXMZdaKBw" } }, "at_hash": "zihakOfULQlcTG-y0xXNyQ" }

Implementation

Besides the usual JWS checks and the WebId derivation, WebId-OIDC requires several additional checks due to the decentralized multi-party nature of the use case. The NSS is the reference implementation for a Solid backend and does the following steps in accordance with the specification (the steps below link to the corresponding code lines in the NSS implementation):

• WebId confirmation
  • extract WebId from claims: take the webid claim, if not present take the sub claim if it is an URL
  • check the issuer claim is present and that the WebId's origin is a sub domain of the issuer domain, including protocol match
• Proof of Possession Checks
  • validate PoP Token
    • check it is a PoP Token
    • verify PoP Token Signature and PoP Issuer matches with audience in Id-Token
    • resolve Provider Key for verification of the Id-Token signature (OIDC Provider Metadata Discovery)
    • verify Id Token Signature
    • verify Expiration of both the PoP and Id-Tokens

Our implementation reassembles these steps in the new WebIdOIDCAuthenticator class.

The WebId-OIDC support is configurable as the other JWT based authentication possibilities. Additionally to the jwt flag the new webIdOIDC node must be defined under the node jwt to activate the feature. This configuration node introduced by the PR has the form:

webIdOIDC:
  enabled: boolean
  cacheSize: int
  cacheExpireDays: int

The cache in this feature is used for the known WebId-OIDC provider keys, fetched on demand according to the OIDC provider metadata discovery.

References:

• https://openid.net/connect/
• https://github.com/solid/webid-oidc-spec

[christophknabe]

Very good. Only some comments and smaller improvement proposals:

• Now exception handling and throwing is good.
• Diagnostic information seems enough.
• These 2 methods seem to appear only together: checkTokenStructure(getTokenParts(xxx)); I think the inner should become inlined or called in the outer method.
• Anyhow the method getTokenParts does overkill. It splits the token String unnecessarily, but is interested only in the number of periods. You should use one of the methods in https://www.baeldung.com/java-count-chars for example the token.chars().filter(ch -> ch == 'e').count(). And more than 2 periods should also be an error.
• The issuer, webId, subject test is difficult to understand, especially as the & | logic appears once in this form, but negated in the exception message.
• I propose to separate the issuer check with its own, then simpler exception message.
• Then the second part would be simpler: if(webId==null && subject==null) with a simpler exception message.
• By the way, what does the Spec say about a claim, where both webId, and subject are given. Is this an error?
• The 2 calls of doSameOriginOrSubDomainConfirmation are redundant. A better solution would be to compute the argument to the call as follows: final String webIdClaim = webId!=null ? webId : subject;

[rodant]

Good comments. I took over all your proposals. Concerning your question about the spec, it says nothing about the case where both claims are present, only about the priority. If the the webid claim is present, it should be used.

[christophknabe]

Very good. Thanks for looking up the WebId spec. So the decision final String webIdClaim = webId!=null ? webId : subject; meets the spec.

[christophknabe]

Introduction Review

• Line 3ff: "a user accesses a resource hosted on Trellis as relying party (RP) and uses its authentication data provided by a Solid client after a login step against a NSS server, as WebId-OIDC provider (OP)."
  There are to my knowledge 3 roles here, but I do not clearly recognize them. For example "as relying party (RP)" could equally relate to "Trellis" or to "a user".
  Here my formulation proposal as I understand the problem with the WebId roles in bold: "A User accesses a resource in his Other POD hosted on Trellis (Resource Server and Relying Party) and uses his authentication data presented by a Solid client (Presenter) after a login step against a NSS server (Identity Provider)."

Code Review

• new URL(location).openConnection().getInputStream() should be coded by the convenience method InputStream openStream(). This formulation would clearly express, that it is not necessary to close the URLConnection, but closing the InputStream is enough.

• "Error fetching/parsing jwk document" should be parameterized with location

• non-static fields baseUrl, keys should be placed after all static parts of the class.

• "Couldn't find key id %s= by provider %s" should avoid the = char and avoid naming the issuer as provider in the error message.

• "Error fetching/parsing WebId-OIDC provider configuration" should be parameterized with issuer.

• Replace mus by must.

• Check for 3 properties too complicate. Better: Split up in 2 checks with specific error messages:

  if (!isIssuerHttpURI || !isWebIdHttpURI) {
      throw new WebIdOIDCJwtException(
           String.format("Issuer or WebId URI is neither http nor https, (iss, webid)=(%s, %s).", issuer, webIdClaim));
  }
  if (!isSubDomainOrSameOrigin) {
      throw new WebIdOIDCJwtException(
           String.format("WebId has neither same domain nor subdomain as issuer, (iss, webid)=(%s, %s).", issuer, webIdClaim));
  }

• "WebId provider confirmation failed, received invalid URI: (iss, sub)=(%s, %s)" should name message parameter webid instead of sub, like in the other messages.
• Test driver looks good.
• Make WebIdOIDCConfiguration immutable. This works with Jackson by constructor injection as in https://www.baeldung.com/jackson-deserialize-immutable-objects. I suppose that you cannot change the configuration of the WebIdOIDC authentication after is has been started. At least in the test driver TrellisUtilsTest I do not see any config.setProperty(...) calls except in the added method testGetJwtAuthenticatorWebIdOIDC with webIdOIDC.setEnabled(true), which can be simply modified.

[rodant]

Thanks for the comments. Concerning the PR description, I agree basically, I'd just wont write "...in his Other POD hosted ...", just "in his POD hosted ...". The user may only have one POD on Trellis and an Identity by the Identity Provider.

[rodant]

I agree with all the code comments, except the last one. I'm not sure whether an immutable design works in our case, because the configuration classes are not only JSON serializable, they must also be read from a YAML configuration supporting property default values. The deserialization from the config file must be able to create the objects in absence of a property using a default value. We could try the approach from here, but I'm not sure whether it makes sense in this PR, because the rest of the configuration classes in Trellis are mutable.

[christophknabe]

Thanks for the consideration of my review. Here my thoughts about your last 2 messages and the new code.

• I agree making WebIdOIDCConfiguration immutable might not be necessary, because configuration is not done in a multi-threaded environment. If it only were about Jackson I am sure, it would work, as I had done it before. About YAML I do not know. So it would need further experimentation and might not pay out.
• I agree on the wording POD instead of Other POD, as it is not necessary an other POD.

Unfortunately I only noticed now, that the SLF4J 1.7.30 API suppresses stack traces when logging a parameterized message with a causing exception.
For example LOGGER.error("Auth failed with issuer {}", issuer, throwable); would invoke the Logger method with the signature void error(String format, Object... arguments) thus treating the Throwable as a normal argument to be inserted into the message format. This would call the Throwable's toString() method and would suppress the stack trace.
In contrary if you want an error method to fully log the stack trace of a Throwable, you would have to call the method with the signature void error(String msg, Throwable t). This variant is not capable of inserting parameters into the message text.

Only SLF4J 2.0 will be capable to combine severity selection, parameter insertion, and an exception by a fluent API. Example:
logger.atDebug().addKeyValue("issuer", issuer).setCause(throwable).log("Auth failed");

If you want to achieve the same including stack trace logging with SLF4J 1.7.30, you have to test the severity and insert the parameters manually in the old style:

if(logger.isDebugEnabled()){
    logger.debug("Auth failed: issuer=" + issuer, throwable);
}

Where are message parameters and exception in logging?

I found one logging statement in the PR, where a causing exception is combined with a parameterized message. If in this place we need the stack trace to be logged, we will have to use the just mentioned manual severity check and message parameterization.

OAuthUtils.fetchKeys

• I do not know, if here the stack trace is needed, although I usually would log it.

[rodant]

Then one solution would be to use error(message, throwable) and to format the message with String.format(format, args).

[christophknabe]

How you format the message is freely electable, but with SLF4J < 2.0 you should include the manual severity checking by if as above, in order to avoid useless string interpolation, if the severity is not configured to be reported. Although I admit this would be more important if severity were DEBUG.

[rodant]

Exactly, I think the error level will be always on and the check it is not very valuable. With the String#format method the exception stack trace will be also logged, right?

[christophknabe]

Yes, then you have only 2 arguments of types (String, Throwable), and the Throwable is not stringized as a normal message argument. Nevertheless I prefer to follow the pattern
if(logger.isErrorEnabled()){logger.error(message, throwable);}.
Reason: Some DevOp could be bothered by too many error messages in the log file and reduce the severity to logger.debug(message, throwable); Then the message creation overhead argument would count again.

[christophknabe]

But it is OK as it is now. If really someone reduces the severity to logger.debug(...) he should insert the manual severity check if(logger.isDebugEnabled()), too (for efficiency).
Overall GREAT work.