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

Add HTTP Client #7

Merged
merged 16 commits into from
Dec 11, 2023
Merged

Add HTTP Client #7

merged 16 commits into from
Dec 11, 2023

Conversation

MicahParks
Copy link
Owner

No description provided.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

Verified

This commit was signed with the committer’s verified signature.
crazy-max CrazyMax

Verified

This commit was signed with the committer’s verified signature.
crazy-max CrazyMax
@MicahParks MicahParks merged commit cb01b09 into master Dec 11, 2023
@MicahParks MicahParks deleted the client branch December 11, 2023 01:19
if err != nil {
return fmt.Errorf("failed to create JWK from JWK Marshal: %w", err)
}
err = store.KeyWrite(options.Ctx, jwk)

Choose a reason for hiding this comment

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

@MicahParks - question on this old code. Shouldn't the refresh routine delete the existing keys that are no longer present in the updated response? KeyWrite seems to append to the existing set, but doesn't remove the keys that are no longer valid. if my initial jwks set had keys k1 and k2 and then I revoke k1 and create k3, refresh routine should add k3 and remove k1. I don't think it is removing k1. Is this intentional?

Copy link
Owner Author

Choose a reason for hiding this comment

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

My expected behavior is the old keys (associated to that specific JWK Set) should be deleted and the new ones written. If it's not doing that, it's a bug, maybe even a security concern for some use cases.

It gets tricky because this implementation handles "given" keys and multiple JWK Sets at once.

I'm going to write up a Proof of Concept (POC) trying to prove what you mentioned. If the POC works, I'll make a bug issue, fix it, and publish a new release.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Here's the POC I promised. Looks like the bug is confirmed. I'll make a ticket.

POC output:

2025/01/08 20:21:17 INFO Old key. kid=836a7fb7-03a3-40cb-ab39-40235ed1de0c
2025/01/08 20:21:17 INFO New key. kid=836a7fb7-03a3-40cb-ab39-40235ed1de0c
2025/01/08 20:21:17 INFO New key. kid=ee966d68-6739-40d3-b652-c7ad023fa9cd

POC code:

package main

import (
	"context"
	"crypto/ed25519"
	"crypto/rand"
	"log/slog"
	"net/http"
	"net/http/httptest"
	"net/url"
	"os"
	"sync"
	"time"

	"github.com/google/uuid"

	"github.com/MicahParks/jwkset"
)

const (
	logErr = "error"
)

func main() {
	ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
	defer cancel()
	l := slog.Default()

	es := newExternalServer(l)
	es.newKeys(ctx)

	server := httptest.NewServer(es)

	u, err := url.ParseRequestURI(server.URL)
	if err != nil {
		l.ErrorContext(ctx, "Failed to parse URL.",
			logErr, err,
		)
		os.Exit(1)
	}

	refreshInterval := time.Second
	options := jwkset.HTTPClientStorageOptions{
		Ctx: ctx,
		RefreshErrorHandler: func(ctx context.Context, err error) {
			l.ErrorContext(ctx, "Failed to refresh keys.")
			os.Exit(1)
		},
		RefreshInterval: refreshInterval,
	}
	client, err := jwkset.NewStorageFromHTTP(u, options)
	if err != nil {
		l.ErrorContext(ctx, "Failed to create HTTP client storage.",
			logErr, err,
		)
		os.Exit(1)
	}

	oldKeys, err := client.KeyReadAll(ctx)
	if err != nil {
		l.ErrorContext(ctx, "Failed to read keys.",
			logErr, err,
		)
		os.Exit(1)
	}

	es.newKeys(ctx)
	time.Sleep(2 * refreshInterval)

	newKeys, err := client.KeyReadAll(ctx)
	if err != nil {
		l.ErrorContext(ctx, "Failed to read keys.",
			logErr, err,
		)
		os.Exit(1)
	}

	for _, oldKey := range oldKeys {
		l.InfoContext(ctx, "Old key.",
			"kid", oldKey.Marshal().KID,
		)
	}
	for _, newKey := range newKeys {
		l.InfoContext(ctx, "New key.",
			"kid", newKey.Marshal().KID,
		)
	}
}

type externalServer struct {
	l     *slog.Logger
	mux   sync.RWMutex
	store jwkset.Storage
}

func newExternalServer(l *slog.Logger) *externalServer {
	e := &externalServer{
		l:     l,
		store: jwkset.NewMemoryStorage(),
	}
	return e
}

func (e *externalServer) ServeHTTP(w http.ResponseWriter, r *http.Request) {
	ctx := r.Context()
	e.mux.RLock()
	defer e.mux.RUnlock()
	raw, err := e.store.JSON(ctx)
	if err != nil {
		w.WriteHeader(http.StatusInternalServerError)
		e.l.ErrorContext(ctx, "Failed to get JWK set as JSON.",
			logErr, err,
		)
		os.Exit(1)
		return
	}
	_, _ = w.Write(raw)
}

func (e *externalServer) newKeys(ctx context.Context) {
	m := jwkset.NewMemoryStorage()
	pub, _, err := ed25519.GenerateKey(rand.Reader)
	if err != nil {
		e.l.ErrorContext(ctx, "Failed to generate new key.",
			logErr, err,
		)
		os.Exit(1)
	}
	jwk, err := jwkset.NewJWKFromKey(pub, jwkset.JWKOptions{
		Metadata: jwkset.JWKMetadataOptions{
			KID: uuid.New().String(),
		},
		Validate: jwkset.JWKValidateOptions{},
		X509:     jwkset.JWKX509Options{},
	})
	if err != nil {
		e.l.ErrorContext(ctx, "Failed to create JWK from key.",
			logErr, err,
		)
		os.Exit(1)
	}
	err = m.KeyWrite(ctx, jwk)
	if err != nil {
		e.l.ErrorContext(ctx, "Failed to write new key to storage.",
			logErr, err,
		)
		os.Exit(1)
	}
	e.mux.Lock()
	e.store = m
	e.mux.Unlock()
}

Copy link
Owner Author

Choose a reason for hiding this comment

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

@rohitkoul do you want credit in the upcoming security advisory I'm about to publish?

Choose a reason for hiding this comment

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

@MicahParks Thank you for looking into this and such a quick turn-around. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants