Implement OIDC4VCI Credential Endpoint #369

andresuribe87 · 2023-03-31T01:58:00Z

Overview

This change implements https://openid.net/specs/openid-4-verifiable-credential-issuance-1_0.html#name-credential-endpoint

Description

In order to support this, some prerequisites were implemented:

A middleware called introspect that can be set on a per handler basis. It's responsible for gating access to the endpoint defined by the handler.
Scaffolding for the endpoint.

The implementation itself includes:

Using a time based one time password for the nonce implementation. The seed key used is the serviceKey already stored in the keyService.
Models for the request and response.

How Has This Been Tested?

Multiple unit tests.

Checklist

Before submitting this PR, please make sure:

I have read the CONTRIBUTING document.
My code is consistent with the rest of the project
I have tagged the relevant reviewers and/or interested parties
I have updated the READMEs and other documentation of affected packages

References

https://openid.net/specs/openid-4-verifiable-credential-issuance-1_0.html#name-credential-endpoint

…lt location.

This reverts commit c3bf59e.

codecov-commenter · 2023-03-31T02:04:36Z

Codecov Report

Merging #369 (2332326) into main (362e4d2) will decrease coverage by 0.01%.
The diff coverage is 43.22%.

@@            Coverage Diff             @@
##             main     #369      +/-   ##
==========================================
- Coverage   20.23%   20.23%   -0.01%     
==========================================
  Files          42       50       +8     
  Lines        4675     4928     +253     
==========================================
+ Hits          946      997      +51     
- Misses       3597     3790     +193     
- Partials      132      141       +9

Impacted Files	Coverage Δ
config/config.go	`16.04% <0.00%> (-0.10%)`	⬇️
pkg/server/router/oidc_credential.go	`0.00% <0.00%> (ø)`
pkg/service/keystore/service.go	`39.35% <0.00%> (-0.52%)`	⬇️
pkg/testutil/setup.go	`0.00% <0.00%> (ø)`
pkg/server/middleware/introspect.go	`63.07% <63.07%> (ø)`
pkg/server/server.go	`80.82% <76.92%> (-0.25%)`	⬇️

... and 6 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

decentralgabe · 2023-03-31T14:42:00Z

pkg/server/middleware/introspect.go

+
+// Introspect extracts a token from the `Authorization` header, and determines whether it's active by using the
+// Endpoint configured. A `nil` error represents an active token.
+func (s introspecter) introspect(ctx context.Context, req *http.Request) error {


nit: i introspector ?

decentralgabe · 2023-03-31T14:42:39Z

pkg/server/middleware/introspect.go

+		err := body.Close()
+		if err != nil {
+			logrus.WithError(err).Warn("closing body")
+		}


Suggested change

err := body.Close()

if err != nil {

logrus.WithError(err).Warn("closing body")

}

if err := body.Close(); err != nil {

logrus.WithError(err).Warn("closing body")

}

decentralgabe · 2023-03-31T14:44:07Z

pkg/server/middleware/introspect.go

@@ -0,0 +1,122 @@
+package middleware


I am wondering if it may be confusing to co-locate server code for both binaries. what do you tink about having a duplicate file structure for oidc stuff? or, alliteratively a set of oidc packages within existing packages?

decentralgabe · 2023-03-31T14:44:42Z

pkg/server/middleware/introspect_test.go

+	return nil
+}
+
+func simpleOauthTokenServer() *httptest.Server {


Suggested change

func simpleOauthTokenServer() *httptest.Server {

func simpleOAuthTokenServer() *httptest.Server {

decentralgabe · 2023-03-31T14:45:04Z

pkg/server/middleware/introspect_test.go

+		ClientSecret:   "",
+		TokenURL:       mockTokenServer.URL,
+		Scopes:         []string{"notsurewhatscope"},
+		EndpointParams: nil,


can remove this and client secret?

decentralgabe · 2023-03-31T14:46:13Z

pkg/server/router/oidc_credential.go

+}
+
+// CredentialError implements https://openid.net/specs/openid-4-verifiable-credential-issuance-1_0.html#name-credential-response
+type CredentialError struct {


OIDCCredentialError?

decentralgabe · 2023-03-31T14:47:28Z

pkg/server/server_oidc_issuance_test.go

+	oidcRouter, err := router.NewOIDCCredentialRouter(oidc.NewOIDCService(
+		didService.GetResolver(),
+		credService,
+		[32]byte{


where's this data coming from?

decentralgabe · 2023-03-31T14:49:38Z

pkg/server/server_oidc_issuance_test.go

+	err = oidcRouter.IssueCredential(newRequestContext(), w, req)
+	require.NoError(t, err)
+
+	var errResp2 map[string]any


nit: favor make for maps

decentralgabe · 2023-03-31T14:49:42Z

pkg/server/server_oidc_issuance_test.go

+	require.Equal(t, 120, credentialResponse.CNonceExpiresIn)
+
+	// And do it again, but after the expiration time. We should now expect an error
+	<-time.After(time.Duration(int64(errorResp["c_nonce_expires_in"].(float64)) * int64(time.Second)))


decentralgabe · 2023-03-31T14:50:50Z

pkg/service/oidc/service.go

+	var subject string
+	var err error
+	switch credRequest.Proof.ProofType {
+	case "jwt":


can we use a known type for this. think we have one in the sdk

decentralgabe · 2023-03-31T14:51:21Z

pkg/service/oidc/service.go

+			Secret:      serviceKey[:],
+		})
+		if err != nil {
+			panic(err)


this should return (*Service, error)

decentralgabe · 2023-03-31T14:53:01Z

pkg/service/oidc/service.go

+		key, err := totp.Generate(totp.GenerateOpts{
+			Issuer:      "ssi-service",


should these values come from config?

decentralgabe · 2023-03-31T14:54:10Z

pkg/service/oidc/service.go

+
+func (s Service) CredentialEndpoint(ctx context.Context, credRequest *model.CredentialRequest) (*model.CredentialResponse, error) {
+	if credRequest.Format == "" {
+		return nil, framework.NewRequestErrorMsg("invalid_request", http.StatusBadRequest)


would consider creating a const block of errors ...

const ( InvalidRequest OurErrorType = "invalid_request" ... )

decentralgabe · 2023-03-31T14:56:29Z

pkg/service/oidc/service.go

+		return nil, errors.New("proof_type not recognized")
+	}
+
+	serviceResp, err := s.credService.GetCredentialsBySubject(ctx, credential.GetCredentialBySubjectRequest{Subject: subject})


this logic wasn't too straightforward for me to understand...we're getting all credentials for a subject, and then comparing the object model to see if it matches the passed in cred?

I think we can consider different paths for credential equality. if the entire object needs to match, can we simplify to the id? What about a triple of id, schema, issuer or issuanceDate? id is probably sufficient...

decentralgabe · 2023-03-31T14:56:44Z

pkg/service/oidc/service.go

+
+	serviceResp, err := s.credService.GetCredentialsBySubject(ctx, credential.GetCredentialBySubjectRequest{Subject: subject})
+	if err != nil {
+		return nil, err


decentralgabe · 2023-03-31T14:57:17Z

pkg/service/oidc/service.go

+	return nil, errors.New("no credential found")
+}
+
+func sameElements(arr1, arr2 []string) bool {


does reflect.DeepEquals work as a replacement for this?

decentralgabe · 2023-03-31T14:57:44Z

pkg/service/oidc/service.go

+	if len(message.Signatures()) != 1 {
+		return "", errors.New("jwt expected to have exactly one signature")
+	}
+	headers := message.Signatures()[0].ProtectedHeaders()


careful to avoid NPE if no signatures

decentralgabe · 2023-03-31T14:58:06Z

pkg/service/oidc/service.go

+	headers := message.Signatures()[0].ProtectedHeaders()
+
+	// - typ: REQUIRED. MUST be openid4vci-proof+jwt, which explicitly types the proof JWT as recommended in Section 3.11 of [RFC8725].
+	const openID4VCIType = "openid4vci-proof+jwt"


could this const be pulled out of the method defn?

decentralgabe · 2023-03-31T14:59:35Z

pkg/service/oidc/service.go

+func (s Service) isNonceValid(nonce string) bool {
+	valid, err := totp.ValidateCustom(nonce, s.key.Secret(), time.Now(), s.validateOpts())
+	if err != nil {
+		logrus.WithError(err).Error("Problem validating")


if there's an err should we return false?
what if valid == true and err == something?

nitro-neal · 2023-04-03T16:21:14Z

pkg/server/middleware/introspect.go

+	}
+}
+
+// Introspect extracts a token from the `Authorization` header, and determines whether it's active by using the


There is no plug in libraries for this?

nitro-neal · 2023-04-03T16:25:53Z

pkg/server/server.go

+func (s *SSIServer) OIDCAPI(service svcframework.Service) error {
+	r, err := router.NewOIDCCredentialRouter(service)
+	if err != nil {
+		return err


Suggested change

return err

return util.LoggingErrorMsg(err, "could not create OIDC router")

nitro-neal

There seems to be a lot low level code parsing url requests and stuff,

Are there any existing libraries we can use to do some of this heavy lifting?

My friend Chad gave me some examples:
Golang OAuth2 Library: https://pkg.go.dev/golang.org/x/oauth2
osin: https://github.com/RangelReale/osin
oauth2_proxy: https://github.com/oauth2-proxy/oauth2-proxy
Go-Guardian: https://github.com/shaj13/go-guardian

andresuribe87 and others added 30 commits March 17, 2023 18:55

Add a simple server that hosts the credential issuer metadata.

6e64557

Move stuff. Make things secure. modules

fcd2631

Made the issuer metadata be user provided via a file that has a defau…

6bc45f9

…lt location.

Simplified to only address the concern of the PR.

c3bf59e

Merge branch 'main' into OSE-438

aa2c208

Fixed module files

d52f4e2

Update go version for golangci-lint

4b5d2ff

Update golangci-lint version

92793ad

Trying with no cache

fd9c1b7

Trying with no cache for golangci-lint, and cache for setting up.

7859382

Skip all

769a780

ssi-sdk is looking very suz

ba074b3

Revert "Simplified to only address the concern of the PR."

d068b07

This reverts commit c3bf59e.

Added the AuthEndpoint

8c0985f

Fix linter

3017fae

Merge branch 'main' into OSE-433

5c83bf4

Better structure.

77be08e

Merge branch 'main' into OSE-433

9f7ff48

Add a simple server that hosts the credential issuer metadata.

6d49df5

Simplified to only address the concern of the PR.

02a247f

Trying with no cache

a771d0b

Trying with no cache for golangci-lint, and cache for setting up.

b6dd7dc

Skip all

62f8f0b

Revert "Simplified to only address the concern of the PR."

0a2ef96

This reverts commit c3bf59e.

Added the AuthEndpoint

a50d773

Fix linter

af475fd

Better structure.

e87bcfc

Implement Credential Endpoint

c3d751f

PR comments

498ee8b

Issue created.

30229af

andresuribe87 added 3 commits March 30, 2023 18:15

Merge branch 'OSE-433' into OSE-435

b8f3a4c

Merge branch 'main' into OSE-435

e23773e

Mod fixing

8ef452c

andresuribe87 requested review from decentralgabe and nitro-neal as code owners March 31, 2023 01:58

package naming

2332326

decentralgabe reviewed Mar 31, 2023

View reviewed changes

pkg/service/oidc/service.go

Secret: serviceKey[:],

})

if err != nil {

panic(err)

Copy link

Member

decentralgabe Mar 31, 2023

Choose a reason for hiding this comment

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

this should return (*Service, error)

decentralgabe reviewed Mar 31, 2023

View reviewed changes

nitro-neal reviewed Apr 3, 2023

View reviewed changes

decentralgabe mentioned this pull request Jul 31, 2023

Support for the DIF JWT Interop profile #624

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement OIDC4VCI Credential Endpoint #369

Implement OIDC4VCI Credential Endpoint #369

andresuribe87 commented Mar 31, 2023

codecov-commenter commented Mar 31, 2023 •

edited

Loading

decentralgabe Mar 31, 2023

decentralgabe Mar 31, 2023

decentralgabe Mar 31, 2023

decentralgabe Mar 31, 2023

decentralgabe Mar 31, 2023

decentralgabe Mar 31, 2023

decentralgabe Mar 31, 2023

decentralgabe Mar 31, 2023

decentralgabe Mar 31, 2023

decentralgabe Mar 31, 2023

decentralgabe Mar 31, 2023

decentralgabe Mar 31, 2023

decentralgabe Mar 31, 2023

decentralgabe Mar 31, 2023

decentralgabe Mar 31, 2023

decentralgabe Mar 31, 2023

decentralgabe Mar 31, 2023

decentralgabe Mar 31, 2023

decentralgabe Mar 31, 2023

nitro-neal Apr 3, 2023

nitro-neal Apr 3, 2023 •

edited

Loading

nitro-neal left a comment

	func simpleOauthTokenServer() *httptest.Server {
	func simpleOAuthTokenServer() *httptest.Server {

		key, err := totp.Generate(totp.GenerateOpts{
		Issuer: "ssi-service",

	return err
	return util.LoggingErrorMsg(err, "could not create OIDC router")

Implement OIDC4VCI Credential Endpoint #369

Are you sure you want to change the base?

Implement OIDC4VCI Credential Endpoint #369

Conversation

andresuribe87 commented Mar 31, 2023

Overview

Description

How Has This Been Tested?

Checklist

References

codecov-commenter commented Mar 31, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nitro-neal Apr 3, 2023 • edited Loading

Choose a reason for hiding this comment

nitro-neal left a comment

Choose a reason for hiding this comment

codecov-commenter commented Mar 31, 2023 •

edited

Loading

nitro-neal Apr 3, 2023 •

edited

Loading