Timeout Caveat Support

[bucko13]

This is a proof of concept to address #80. It is loosely inspired by macaroontimeout and TimeoutConstraint in lnd.

The way the api for this is supposed to work is that if you want your minted lsats to have an "expiration" for specific services, you set a timeout property in your aperture config at the service level (same as capabilities and constraints). This should be a value that equates to the number of seconds after minting that the lsat should expire. When the lsat is minted, this value will be consumed and the unix timestamp for that number of seconds from that moment will be added as the value for a caveat with the condition [service_name]_timeout.

I've tested this out in an application I'm working on for which this is a feature requirement. If this isn't the preferred approach, I'd be happy to collaborate to find one that works or do what needs to be done to make this an official part of aperture.

[bucko13]

For reference I have a PR that adds a satisfier for validating this caveat on LSATs for the service I'm aiming to use this for. See IsExpiredSatisfier here

[ellemouton]

Hi @bucko13, thanks for the PR!

Some questions I have: why not just use the existing macaroon timeout caveat functions as is done in lncli? And also, I think that if we add specific conditions to aperture itself, then we also need to add the checks for those conditions in aperture. Because if I understand correctly, this PR just adds a new condition to the lsat but expects the downstream server to verify that a request is valid/invalid given the condition?

[bucko13]

@ellemouton im not sure I totally understand. Is aperture used in lncli or between access from lncli and lnd? My understanding is that this is a proxy server used in a totally different type of environment (more like between client server) than between more of a “master” admin (lncli) talking with the tool you want to control access to (lnd). In fact, I based this caveat tooling on what I assume is used in lncli.

[ellemouton]

@bucko13 , I just meant that as an example: like lncli uses a standard caveat name for the timeout that it gets from the macaroon lib. That way lnd can use the existing expiry check from the lib too.

With how this PR is currently - how I understand it is that it is adding a completely new caveat name to the macaroon and then when a request comes through with a macaroon that has that caveate, aperture itself is not checking the validity of the caveat but is rather depending on the backend service to know how to do that?

[bucko13]

Ah, yeah sure I can change the name. Everything else still basically needs to be done after the caveat goes through as far as I understand the way aperture is constructed. Aperture right now can't verify that all of the services that you add on are valid, that has to be done after the base validation is done and the request has been passed through.

Here's an example in a tech stack where I'm trying to use aperture as a proxy, where I have a custom "sphinx-meme" service and all the limitations associated with that for requests. Aperture has now way to use the restrictions on its own, so that's just done in sphinx meme after the request has been proxied.

The same would more or less have to happen with this timeout setting whether or not it's named differently, though it does make it easier to use the standard lib for leveraging it (as lnd presumably does). It's pretty simple to implement the satisfier though: 48 lines for the satisfier and then pass it in to a verifier like this: VerifyCaveats(caveats, NewUploadSizeSatisfier(int64(handler.Size)), IsExpiredSatisfier(time.Now().Unix())).

Do you happen to know what I would need to change the name to so that it follows the standard? I've seen a few. There's this library that was based on the reference (but has diverged since I think), but they use time in their examples. In the go reference I don't see anything obviously documented but I could be missing it.

[bucko13]

@ellemouton I just wanted to bump this one more time to see if it's something that could be supported in the official aperture repo so we don't have to rely on a fork.

I'd like to try and understand better what the main concern with this approach is. If it's simply that the external service needs to validate the caveat on its own, I can understand why that might be a problem however I'd just point out that as best I can see this is more or less how aperture is expected to function in most cases.

Consider one of the examples in the lsat.tech service constraints example where there's a caveat that looks like loop_out_monthly_volume_sats = 200000000. As best as I can tell, there's no way that aperture can know if this volume has been surpassed or not and it will have to be checked by the service on the backend anyway. In fact, for aperture to have any sort of flexibility most of the validation beyond just restricting access to specific paths is going to have to be done after aperture has done a surface level validation of the macaroon.

That said, I see no reason why I couldn't build on this PR to also add support to also validate the timeout validation (which incidentally as I mentioned you already do in lncli). The only reason I didn't is because I thought it would be easier to keep the scope as reduced as possible and since this is how other caveats seem to work already it seemed reasonable to me. If I did add the validation as well, I'd like to try and get as clear as possible of an idea of what the expectations are for something like this. If it makes sense to you, I could simply move the satisfier and tests that I have in the other project that leverages this functionality.

In my opinion though, I do think that this is a required type of functionality for something like aperture to support considering so many macaroon and lsat projects have it built in including lncli, macaroon.js, boltwall, and aperture hints at it with the sample config although the absolute (as opposed to relative) construction doesn't seem usable in many production use cases.

[ellemouton]

Hey @bucko13 - sorry for the late reply, I have been OOO for a while.

As best as I can tell, there's no way that aperture can know if this volume has been surpassed or not and it will have to be checked by the service on the backend anyway. In fact, for aperture to have any sort of flexibility most of the validation beyond just restricting access to specific paths is going to have to be done after aperture has done a surface level validation of the macaroon.

Yes but the difference is that in that example, aperture doesnt know what the constraint is that it is adding but in this timeout case, it does. In this PR it is even classified in a different category to the other static constraints. with the other constraints aperture blindly adds them to the macaroon as caveats and it is the responsibility of the user to make sure that the backend server will know how to verify the caveat condition. But if we are making a whole new Timeout field - it is not clear that the user needs to make sure that the backend service should know how to verify that the macaroon has/has not timed out. So I think that if aperture is going to handle this timeout case specifically, then it should also verify the caveat.

Im also wondering - is this the only dynamic caveat that we may want? Ie, is there perhaps a future where we want multiple different dynamic caveats? In that case - what would be cool is to have a caveat name in the config & to indicate that it is a dynamic caveat. Then aperture will know to ask the backend service what to add as a caveat at the time of macaroon creation. This would make things more generic and then it would be fine to let the backend service verify the timeout caveat etc.

Thoughts?

(ps: I will try reply faster from now on 😅)

[bucko13]

Hi @ellemouton, now it's my turn to apologize for the delayed response!

But if we are making a whole new Timeout field - it is not clear that the user needs to make sure that the backend service should know how to verify that the macaroon has/has not timed out. So I think that if aperture is going to handle this timeout case specifically, then it should also verify the caveat.

Yep, I agree that aperture would be the best place for the verification. Since there was no other verification happening in aperture beyond the cryptographic macaroon verifications, I thought there might've been a reason for leaving others out, including the valid_until in the sample conf. But if that's something you'd be ok accepting into the project, I'd be happy to move the validation code I [wrote in sphinx](https://github.com/stakwork/s
phinx-meme/pull/10/files#diff-28ef562dd4a2fb366dc11e80f6daa38220b2d132d2878c752d2f67761f4fbc6fR87-R135) over here.

As for generic dynamic caveats, to just try and understand the proposal better, you're suggesting maybe NOT having the verification in aperture but rather having a way to tell aperture that you want to support a dynamic caveat that will be generated and provided by the backend service which would then be responsible for verifying them?

So, something like this in the config.yml:

dynamiccaveats:
    host: "127.0.0.1:8888"
    path: "/dynamic-caveats"

Then when the macaroon is baking, if this setting is on, aperture will call GET 127.0.0.1:8888/dynamic-caveats and get back some response that looks like:

{
     "caveats": ["timeout=1670903314095", "challenge=e852397d-dd33-4808-b98e-2ee59b4cf5fb"] 
}

and then it can just be assumed that if a backend service is providing the caveats then it can validate them. (Note, I just came up with this off the top of my head. Definitely happy to workshop what the API looks like)

I think this seems reasonable to me. I'm trying to think of the best way to keep it nested and associated with a particular service, i.e. still support service2_timeout=1670903314095 without making the API too complex.

The only other downside of this is that it does significantly (imo) complicate the requirements for a backend service that needs/wants to leverage this. The nice thing about doing entirely via macaroons is that while the backend service does have to be able to validate the caveats, that's a pretty universal API. Knowing how to send and receive messages in a way aperture can understand is more complex than just having the information embedded in a standardized format in the Header. And then you also have to take into account compatibility, basically versioning the api the two servers use to talk to each other in case something changes.

Really the best solution imo would be some kind of plugin and/or middleware system where rather than telling aperture "talk to this endpoint to figure out what other caveats to add", you would tell aperture: "load this set of middleware to add and verify custom caveats". So this makes it configurable and also keeps the verification and caveat generation in the same place.

In fact, this is basically what I built for the Nodejs version of aperture, Boltwall. It's a little different since Boltwall is a middleware rather than a proxy server, but the idea is that when you add boltwall to your server, you can pass in custom configs when you initialize the middleware. One of the built-in configs that comes with boltwall is even a timeout caveat exactly like what we're talking about!

Designing a plugin system for aperture though would obviously be a much more complex project to build.

[ellemouton]

Designing a plugin system for aperture though would obviously be a much more complex project to build.

completely agree. I think this would be a future feature :) I think valid_until is generic enough that aperture can just handle it. So yeah, if you want to move your validation code into aperture as a PR then that would be great!

[bucko13]

Fantastic! My plate has been clearing up after a very busy few months so I’ll try and get an update up soon.

as an aside, a plug-in system would be REALLY cool and very appropriate for a system like this, but yeah, out of scope.

[ellemouton]

Fantastic! My plate has been clearing up after a very busy few months so I’ll try and get an update up soon.

Agreed!!!

as an aside, a plug-in system would be REALLY cool and very appropriate for a system like this, but yeah, out of scope.

Completely agree!! Im hopeful that more people/groups will realise the value of LSATs soon which will give us more incentive to sink some time into cool things like that

[bucko13]

@ellemouton take a look! the timeout caveat (which is set as service suffix + _valid_until) is now validated by aperture as well and there are tests for the satisfier and the minter's verifier.

[ellemouton]

Awesome @bucko13 ! Before I jump into review, do you mind consolidating the commits a bit? for example, there are commits where you add things but then remove/change/format them in later commits. It will make reviewing a lot easier and is in any case what will be needed before merging :) Thanks!

[bucko13]

@ellemouton no problem. Hope this is better!

[bucko13]

Bumping this on request of the bot

[ellemouton]

Very Cool PR & great test coverage!! 🔥

Left quite a few comments and suggestions :)

Gonna be great to have this functionality!

[bucko13]

Thanks for the review @ellemouton! will push up some updates asap. Quick question as a relatively new Golang developer- do you have any suggestions for setting up local formatting issues like the ones you commented on? I have the built in linter from VSCode, and used go fmt but obviously it didn't catch some rules for the code base.

[guggero]

Unfortunately there is no good formatter (that I know of) that does this the same way prettier would for JS/TS automatically. It's the one thing the Golang ecosystem is missing IMO. We can tune the linter to be more pedantic about formatting things, but that doesn't solve the main issue of fixing it automatically, unfortunately.

[ellemouton]

yeah - @bucko13 , I think the best would be to read over the LND contribution guidelines
Also, seems like lots of the linter issues here could be solved by running go fmt in the same commit as the code is added in :)

[bucko13]

Also, seems like lots of the linter issues here could be solved by running go fmt in the same commit as the code is added in :)

Hmmm I'm pretty sure I did that but maybe not. Since all of this is basically just one feature, would you have any objection to my just applying go fmt, addressing all the remaining issues, and then just squashing into a single commit?

(sorry got pulled off in other directions again. I'm determined to wrap this up this week though if possible and will find the time assuming nothing really crazy comes up in the meantime)

[ellemouton]

HI @bucko13 - yeah I think the diff is small enough & all the parts fit together quite tightly so having them in one commit should be fine :)

[bucko13]

For autoformatting with vscode I actually found the combination of golines plus the runonsave extension to work pretty well in helping me clean things up a little easier.

[bucko13]

Thank you for your patch and your patience elle! Very nice cleanup. Hopefully everything's been addressed now.

[ellemouton]

Getting there!

[ellemouton]

not required anymore - also results in a compile error

[ellemouton]

nit: since this is a returned error, this should be formatted like this:

	return fmt.Errorf("not authorized to access " +
				"service. LSAT has expired")

[bucko13]

Just noting that this was from your patch. It would be really nice if there was a way to get repeatable and predictable automation for rules like this. As I noted in another comment, there are some places in already existing code (which is what I, as a new contributor, will default to as a reference) that doesn't follow a lot of these rules.

[ellemouton]

imports should be: first standard lib imports, new line, other imports. So time belongs in the standard section and the new line should stay

[ellemouton]

nit: remove new line below

[ellemouton]

same comment re imports: math/rand belongs in the standard lib section and new line should stay

[bucko13]

I’d really recommend just committing to a line length formatted even if it has some undesirable outcomes. It’s pretty inconvenient to have to check every single line manually, which I was already trying to do on the patch that was provided but apparently missed some (I did push some changes that were missed in the patch too so a lot is falling through the cracks). These rules are also not even fully implemented across the repo as it is as I found several other lines not from my diff when trying to fix the ones mentioned here but didn’t want to fix out of concern of expanding scope of the commits

[bucko13]

Ok, should be addressed now. There are fewer remaining comments so it was easier to keep track without marking as resolved so I left them all unresolved to avoid any other mixups. Also rebased with master.

[ellemouton]

Thanks for the fast updates @bucko13! 🚀 Soooo close - just checkout the remaining linter error: basically I think you just need to add a test := test line in the satisfier test for loop that iterates over the tests (it's a weird go thing)

Yeah - sorry about the line length things - the older code in the repo doesnt match the standard that we try stick to now 🤷‍♀️ I know it can be annoying - but thanks for your patience & fast updates :)

[bucko13]

Nice, found this on that linting error.

[bucko13]

Yay, green checks!

[lightninglabs-deploy]

@ellemouton: review reminder
@bucko13, remember to re-request review from reviewers when ready

[ellemouton]

Excellent work @bucko13!! tACK! 🔥

The last thing that should be added is a timeout entry in the sample-conf.yaml file.

Once that is added, then this LGTM! 🚀

[guggero]

Nice feature, thank you! LGTM pending @ellemouton's last comment and my nits.

[bucko13]

Thanks @ellemouton!

The last thing that should be added is a timeout entry in the sample-conf.yaml file.

Added this. One question I have is that there's currently an example constraint value added in the sample that is kind of made obsolete from this:

    # The set of constraints that are applied to tokens of the service at the
    # base tier.
    constraints:
        "valid_until": "2020-01-01"

This was obviously just for example purposes anyway so it's not a huge deal, but it is something y'all are sharing for an example so I didn't want to remove. Happy to change it to something else if there was an example from your own internal usage you thought would be better, or I could just leave it.

[ellemouton]

good catch - I think we can leave it for now since it is not exactly the same. The "valid_until" is an absolute timeout whereas the new "timeout" is relative. I think there could be situations where a user would want both. But on the other hand, yes it is just an example so not such a big deal to change it.

[guggero]

The timeout is created by specifying a relative value, yes. But for things to work it still needs to be an absolute timestamp. So I guess the only difference between the example "valid_until": "2020-01-01" and the implemented _valid_until suffix is the format of the timestamp, which would now be a Unix timestamp.
So I do think we should just update the example to include a Unix timestamp instead of the yyyy-mm-dd example value and call it a day.

[bucko13]

So I do think we should just update the example to include a Unix timestamp instead of the yyyy-mm-dd example value and call it a day.

ok so you’re saying to update the valid_until example to be a timestamp as well but just for the corresponding date? To be clear, as far as I know it’s just an example right and doesn’t actually do anything in aperture today?

[guggero]

ok so you’re saying to update the valid_until example to be a timestamp as well but just for the corresponding date? To be clear, as far as I know it’s just an example right and doesn’t actually do anything in aperture today?

Correct. That was just an example of how Aperture could be extended with functionality, which this PR now did. Feel free to use whatever example timestamp you like.

[bucko13]

So I do think we should just update the example to include a Unix timestamp instead of the yyyy-mm-dd example value and call it a day.

Added!