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

multi: change LSAT name to L402 #135

Merged
merged 11 commits into from
Apr 23, 2024
Merged

Conversation

starius
Copy link
Contributor

@starius starius commented Apr 16, 2024

LSAT protocol has been renamed to L402.

This PR replaces all occurrences of "LSAT" with "L402" in the codebase.

Notes:

  • The broken link in the README referring the standard's page, now directs to L402's GitHub repo
  • Both the server and the client previously sent WWW-Authenticate: LSAT macaroon= and Authorization: LSAT ... headers respectively. The protocol defines that they should both use the L402 auth-scheme instead of LSAT. Now, both of them send two headers: first, the LSAT version they used to send before, then the L402 version of the header. The latter contains the same content, but L402 is used as the auth-scheme instead of LSAT. The reason for preserving LSAT versions of the headers and sending them first is because existing instances of clients and servers check the first instance of a header. After Aperture is upgraded, we can stop sending Authorization: LSAT ..., but WWW-Authenticate: LSAT ... will remain at least until all currently deployed loop versions reach the end of life.
  • Unrelated fix: don't send the client's headers back. The Aperture server used to take the request's headers as a basis for building response headers. This behavior has been replaced by creating a fresh header set, initially containing just one header, which is required for things to work: Content-Type: application/grpc.
  • Unrelated fix: optimized away unnecessary regexp MatchString call. It is followed by FindStringSubmatch which does the same job and returns nil in case nothing matches. This results in the same outcome if the removed MatchString call returns false.

auth: LsatAuthenticator -> L402Authenticator
sed -i 's/LsatAuthenticator/L402Authenticator/g' aperture.go auth/authenticator.go auth/authenticator_test.go

rename package lsat to l402
git mv lsat/ l402
sed 's@aperture/lsat@aperture/l402@g' -i `git grep -l aperture/lsat`
sed -i 's@package lsat@package l402@' `git grep -l 'package lsat'`
sed -i 's@lsat\.@l402.@g' -i `git grep -l 'lsat\.'`
sed 's@l402.Id@lsat.Id@' -i mint/mint_test.go

replace lsat with l402 in the code
sed 's@lsat@l402@' -i mint/mint_test.go
sed 's@Lsat@L402@' -i l402/client_interceptor.go
sed 's@lsatstore@l402store@' -i l402/store_test.go

replace LSAT to L402 in comments
sed '/\/\//s@LSAT@L402@g' -i `git grep -l '//.*LSAT'`

replace LSAT -> L402 in the code, skip when a string starts with it
sed 's@\([^"/]\)LSAT@\1L402@g' -i `git grep -l LSAT`
Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

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

Tested backward compatibility with old Loop client.
Looks good, but main change request is around existing token files for existing users/clients.

@@ -15,19 +15,19 @@ var (

// storeFileName is the name of the file where we store the final,
// valid, token to.
storeFileName = "lsat.token"
storeFileName = "l402.token"
Copy link
Member

Choose a reason for hiding this comment

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

If we want to rename this, then I think we'd also need a migration. Otherwise every node that has ever paid for a Loop or Pool token will need to buy a new one (and any discounts offered to customers identified by their token ID will be lost).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great catch! I added migration in a separate commit (l402: migration of lsat.token* files).

base64.StdEncoding.EncodeToString(macBytes), paymentRequest)
header := r.Header
header.Set("WWW-Authenticate", str)
// Old loop software (via ClientInterceptor code of aperture) looks
Copy link
Member

Choose a reason for hiding this comment

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

nit: always add newline before comment block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@@ -71,6 +71,11 @@ func (l *L402Authenticator) Accept(header *http.Header, serviceName string) bool
return true
}

const (
lsatAuthScheme = "LSAT"
Copy link
Member

Choose a reason for hiding this comment

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

nit: some Godoc comments explaining the difference of these two would be useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed:

const (
        // Outdated RFC 7235 auth-scheme used by aperture.
        lsatAuthScheme = "LSAT"

        // Current RFC 7235 auth-scheme used by aperture.
        l402AuthScheme = "L402"
)

Copy link
Member

Choose a reason for hiding this comment

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

One final nit: for a comment to be counted as a Godoc comment, it needs to start with the exact variable/field/method/function name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

auth/authenticator.go Show resolved Hide resolved
l402/header.go Show resolved Hide resolved
@starius
Copy link
Contributor Author

starius commented Apr 22, 2024

@guggero Thank you very much for the review! Great catch of file renames! I addressed the feedback.

@starius starius requested a review from guggero April 22, 2024 19:53
Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

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

One final nit, otherwise LGTM, nice work 💯

@@ -71,6 +71,11 @@ func (l *L402Authenticator) Accept(header *http.Header, serviceName string) bool
return true
}

const (
lsatAuthScheme = "LSAT"
Copy link
Member

Choose a reason for hiding this comment

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

One final nit: for a comment to be counted as a Godoc comment, it needs to start with the exact variable/field/method/function name.

l402/store_test.go Show resolved Hide resolved
Old clients expect "L402 macaroon=..." in the first WWW-Authenticate, while
the protocol [1] says it should be "WWW-Authenticate: L402 macaroon=...",
so send both LSAT and L402. LSAT must be sent first, to maintain backward
compatibility with older clients.

[1] https://github.com/lightninglabs/L402/blob/master/protocol-specification.md
Again, as with WWW-Authenticate header, existing aperture instances
expect LSAT, and the protocol defines it is L402, so send both, LSAT
first, to maintain backward compatibility.

The header "Authorization: LSAT..." can be removed in the future,
when all aperture instances are upgraded.
Create fresh http.Header object filled with the only header:
"Content-Type: application/grpc".
The same regexp is then used in a call of FindStringSubmatch method
and the outcome is the same, if nothing is found there.
This is needed to preserve old identifiers of nodes that have ever paid
for a Loop or Pool token.
@starius
Copy link
Contributor Author

starius commented Apr 23, 2024

Thank you! Fixed the final nit :-)

Copy link
Member

@bhandras bhandras left a comment

Choose a reason for hiding this comment

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

LGTM, really nice work @starius 🥇

@guggero guggero merged commit 9463266 into lightninglabs:master Apr 23, 2024
5 checks passed
@starius starius deleted the s-lsat-l402 branch April 23, 2024 14:48
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

3 participants