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

Wire ACME extensions #1666

Merged
merged 182 commits into from
Aug 9, 2024
Merged

Wire ACME extensions #1666

merged 182 commits into from
Aug 9, 2024

Conversation

hslatman
Copy link
Member

@hslatman hslatman commented Jan 8, 2024

No description provided.

stefanwire and others added 30 commits January 8, 2024 20:27
…_token. Also have a different mapping for id_token claims name
@hslatman
Copy link
Member Author

hslatman commented Mar 6, 2024

@maraino about the DB interface: I tried it before, but without changing a lot more, you'll end up with WireDB being used as the type in every struct, function and method that has to operate on the current DB interface, which I considered worse than just having these methods defined in the interface, and would basically result in what exists today.

This would've been a nice solution that I think can work, but isn't allowed in Go (today, at least; ref):

type DB interface {
    // DB methods
}
type WireDB interface {
    DB 
    // Wire methods
}
type NewDB interface {
    DB | WireDB
}

Alternatives would be:

  • Have the top level DB interface provide all methods, as it does now, and check at runtime that all(/only) the required methods are available, which could be based on some conditional logic taking into account the enabled challenge types. But that way it would still take the implementer to at least define the method in their implementation, so it's not a great solution.
  • Make the methods in the DB interface depend on a build tag.
  • Others ...?

It's not the cleanest solution, but implementers could always return some "not implemented" error. It's not as strong of a guarantee as having an interface, but it works.

So, time for the build tag?

@hslatman hslatman modified the milestones: v0.26.0, v0.26.1 Mar 29, 2024
@hslatman hslatman modified the milestones: v0.26.1, v0.26.2 Apr 25, 2024
@hslatman hslatman modified the milestones: v0.26.2, v0.26.3 Jun 17, 2024
@hslatman hslatman modified the milestones: v0.27.0, v0.27.2 Jul 15, 2024
@hslatman hslatman modified the milestones: v0.27.2, v0.27.3 Jul 23, 2024
@hslatman hslatman requested a review from maraino August 6, 2024 18:27
Copy link
Contributor

@maraino maraino left a comment

Choose a reason for hiding this comment

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

I've added a few comments, but it is generally ok. We need to figure out the WireDB thing sooner rather than later.

Comment on lines +432 to +437
Name string `json:"preferred_username,omitempty"`
Handle string `json:"name"`
Issuer string `json:"iss,omitempty"`
GivenName string `json:"given_name,omitempty"`
KeyAuth string `json:"keyauth"`
ACMEAudience string `json:"acme_aud,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

omitempty tags are not used on unmarshaling.

Comment on lines +475 to +486
orders, err := db.GetAllOrdersByAccountID(ctx, ch.AccountID)
if err != nil {
return WrapErrorISE(err, "could not retrieve current order by account id")
}
if len(orders) == 0 {
return NewErrorISE("there are not enough orders for this account for this custom OIDC challenge")
}

order := orders[len(orders)-1]
if err := db.CreateOidcToken(ctx, order, transformedIDToken); err != nil {
return WrapErrorISE(err, "failed storing OIDC id token")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Although we already discussed this, these methods are problematic. Without implementing them, we won't be able to upgrade the certificates' version.

We should split this into a different interface and return an error if the database doesn't implement it.

We can change this later, but not too late. This should not be hard to change as this method is probably called from Challenge.Validate

}

order := orders[len(orders)-1]
if err := db.CreateDpopToken(ctx, order, map[string]any(*dpop)); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same issue here.

Comment on lines +11 to +22
type UserID struct {
Name string `json:"name,omitempty"`
Domain string `json:"domain,omitempty"`
Handle string `json:"handle,omitempty"`
}

type DeviceID struct {
Name string `json:"name,omitempty"`
Domain string `json:"domain,omitempty"`
ClientID string `json:"client-id,omitempty"`
Handle string `json:"handle,omitempty"`
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It's weird having omitempty tags when most of those values are required later.

@@ -6,6 +6,7 @@ require (
cloud.google.com/go/longrunning v0.5.11
cloud.google.com/go/security v1.17.4
github.com/Masterminds/sprig/v3 v3.2.3
github.com/coreos/go-oidc/v3 v3.4.0
Copy link
Contributor

Choose a reason for hiding this comment

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

It is not the latest version, but I guess ok.

@hslatman hslatman merged commit 8c14b8b into master Aug 9, 2024
14 checks passed
@hslatman hslatman deleted the wire-acme-extensions branch August 9, 2024 21:49
@hslatman hslatman linked an issue Aug 9, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs triage Waiting for discussion / prioritization by team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New provisioner or add to ACME provisioner for Wire™ cert enrollment
5 participants