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.

Sign up for GitHub

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

Jump to bottom

Accept comma separated macaroons #146

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Accept comma separated macaroons #146

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion auth/authenticator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,12 +41,15 @@ func (l *L402Authenticator) Accept(header *http.Header, serviceName string) bool
// Try reading the macaroon and preimage from the HTTP header. This can
// be in different header fields depending on the implementation and/or
// protocol.
mac, preimage, err := l402.FromHeader(header)
macs, preimage, err := l402.FromHeader(header)
if err != nil {
log.Debugf("Deny: %v", err)
return false
}

// TODO: Be able to accept multiple macaroons
Copy link
Member

Choose a reason for hiding this comment

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

nit: add full stop to end of sentence. Here and in server_interceptor.go.

mac := macs[0]

verificationParams := &mint.VerificationParams{
Macaroon: mac,
Preimage: preimage,
Expand Down
48 changes: 32 additions & 16 deletions l402/header.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"fmt"
"net/http"
"regexp"
"strings"

"github.com/lightningnetwork/lnd/lntypes"
"gopkg.in/macaroon.v2"
Expand Down Expand Up @@ -42,7 +43,7 @@ var (
//
// If only the macaroon is sent in header 2 or three then it is expected to have
// a caveat with the preimage attached to it.
func FromHeader(header *http.Header) (*macaroon.Macaroon, lntypes.Preimage, error) {
func FromHeader(header *http.Header) (macaroon.Slice, lntypes.Preimage, error) {
var authHeader string

switch {
Expand Down Expand Up @@ -71,14 +72,22 @@ func FromHeader(header *http.Header) (*macaroon.Macaroon, lntypes.Preimage, erro
macBase64, preimageHex := matches[2], matches[3]
macBytes, err := base64.StdEncoding.DecodeString(macBase64)
if err != nil {
return nil, lntypes.Preimage{}, fmt.Errorf("base64 "+
"decode of macaroon failed: %v", err)
// macBase64 might be multiple macaroons separated by commas,
Copy link
Member

Choose a reason for hiding this comment

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

nit: comments should always be full sentences, including punctuation.

I also think this needs some more explanation that we're using macaroon.Slice below, which can actually decode multiple macaroons that are in one continuous blob of data.

// so we strip them and try again
macBase64 = strings.ReplaceAll(macBase64, ",", "")
macBytes, err = base64.StdEncoding.DecodeString(macBase64)
if err != nil {
return nil, lntypes.Preimage{},
Copy link
Member

Choose a reason for hiding this comment

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

nit: please break the lines before 80 characters, while configuring your IDE to show the tabulator character as 8 spaces (most of the code in this block is now too long).

fmt.Errorf("base64 decode of macaroon failed: %v", err)
}
}
mac := &macaroon.Macaroon{}
// macBytes can contain multiple macaroons,
Copy link
Member

Choose a reason for hiding this comment

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

nit: add empty line before comment. Also missing full stop.

// (* macaroon.Slice) knows how to decode them without separators
mac := make(macaroon.Slice, 0, 1)
err = mac.UnmarshalBinary(macBytes)
if err != nil {
return nil, lntypes.Preimage{}, fmt.Errorf("unable to "+
"unmarshal macaroon: %v", err)
return nil, lntypes.Preimage{},
fmt.Errorf("unable to unmarshal macaroon: %v", err)
Copy link
Member

Choose a reason for hiding this comment

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

Why this formatting change here and below?

}
preimage, err := lntypes.MakePreimageFromStr(preimageHex)
if err != nil {
Expand Down Expand Up @@ -107,32 +116,39 @@ func FromHeader(header *http.Header) (*macaroon.Macaroon, lntypes.Preimage, erro
// extract the preimage.
macBytes, err := hex.DecodeString(authHeader)
if err != nil {
return nil, lntypes.Preimage{}, fmt.Errorf("hex decode of "+
"macaroon failed: %v", err)
return nil, lntypes.Preimage{},
fmt.Errorf("hex decode of macaroon failed: %v", err)
}
mac := &macaroon.Macaroon{}
mac := make(macaroon.Slice, 0, 1)
Copy link
Member

Choose a reason for hiding this comment

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

I know there isn't a unit test now, but it would be really nice to add one so this could be tested.

err = mac.UnmarshalBinary(macBytes)
if err != nil {
return nil, lntypes.Preimage{}, fmt.Errorf("unable to "+
"unmarshal macaroon: %v", err)
}
preimageHex, ok := HasCaveat(mac, PreimageKey)
if !ok {
return nil, lntypes.Preimage{}, errors.New("preimage caveat " +
"not found")
var preimageHex string
Copy link
Member

Choose a reason for hiding this comment

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

nit: multiple var statements can be combined into one:

var (
    ...
)

var ok bool
for _, m := range mac {
Copy link
Member

Choose a reason for hiding this comment

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

I think if there are multiple macaroons, then all of them should have the caveat. Not just one of them. Otherwise IMO the whole idea of having multiple macaroons is weird.

if preimageHex, ok = HasCaveat(m, PreimageKey); ok {
break
}
}
if preimageHex == "" || !ok {
Copy link
Member

Choose a reason for hiding this comment

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

We probably also want to make sure all macaroons reference the same preimage.

return nil, lntypes.Preimage{},
errors.New("preimage caveat not found")
}

preimage, err := lntypes.MakePreimageFromStr(preimageHex)
if err != nil {
return nil, lntypes.Preimage{}, fmt.Errorf("hex decode of "+
"preimage failed: %v", err)
return nil, lntypes.Preimage{},
fmt.Errorf("hex decode of preimage failed: %v", err)
}

return mac, preimage, nil
}

// SetHeader sets the provided authentication elements as the default/standard
// HTTP header for the L402 protocol.
func SetHeader(header *http.Header, mac *macaroon.Macaroon,
func SetHeader(header *http.Header, mac macaroon.Slice,
preimage fmt.Stringer) error {

macBytes, err := mac.MarshalBinary()
Expand Down
5 changes: 4 additions & 1 deletion l402/server_interceptor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,11 +92,14 @@ func tokenFromContext(ctx context.Context) (*TokenID, error) {
}
log.Debugf("Auth header present in request: %s",
md.Get(HeaderAuthorization))
macaroon, _, err := FromHeader(header)
macaroons, _, err := FromHeader(header)
if err != nil {
return nil, fmt.Errorf("auth header extraction failed: %v", err)
}

// TODO: Be able to accept multiple macaroons
macaroon := macaroons[0]

// If there is an L402, decode and add it to the context.
identifier, err := DecodeIdentifier(bytes.NewBuffer(macaroon.Id()))
if err != nil {
Expand Down
Loading