Implement OIDC4VCI Credential Endpoint by andresuribe87 · Pull Request #369 · TBD54566975/ssi-service

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

+
+// 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

+		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

@@ -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

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


Suggested change

func simpleOauthTokenServer() *httptest.Server {

func simpleOAuthTokenServer() *httptest.Server {

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

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


can remove this and client secret?

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

+}
+
+// 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

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


where's this data coming from?

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

+	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

+	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

+	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

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


this should return (*Service, error)

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

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


should these values come from config?

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

+
+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

+		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

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


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

+	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

+	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

+	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

+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

+	}
+}
+
+// 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

+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

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

	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")

Conversation

andresuribe87 commented Mar 31, 2023

Overview

Description

How Has This Been Tested?

Checklist

References

Uh oh!

codecov-commenter commented Mar 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nitro-neal Apr 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nitro-neal left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-commenter commented Mar 31, 2023 •

edited

Loading

nitro-neal Apr 3, 2023 •

edited

Loading