❗ Preview Environment deployment failed on Bunnyshell

See: Environment Details | Pipeline Logs

Available commands (reply to this comment):

• 🚀 /bns:deploy to redeploy the environment
• ❌ /bns:delete to remove the environment

Do we need to be concerned with security implications? For example, might an AppSet be able to retrieve a response that happens to be available in the cache that it wouldn't be able to access if it had to go to GitHub to auth? Do we need the cache to be segmented by AppSet or secret or something like that?

@crenshaw-dev

Do we need to be concerned with security implications?

The caching library used here handle the segmentation internally
The library inject caculate the expected Etag value based on the request headers including Authorization.
So the request is made with the actual Authorization header and if the response is not 304 Not Modified, the response cached is not used (401, 403)

IMO the security questions to consider are more:

• on the library dependency, do we want to have an external dependency for that?
• if there is vulnerability in the ArgoCD that allow an attacker to dump the cache content, do we want to protect against that?

Do we need the cache to be segmented by AppSet or secret or something like that?

Could be an option but it will improve the performance more than the security the Etag recalculation will happen a bit less.

can we also add metrics?

@ppapapetrou76 will add the metrics layer later once other remarks will be solved

The library inject caculate the expected Etag value based on the request headers including Authorization.

I'm a bit worried about this actually. The library is pretty clear that it does this by having reverse-engineered the Etag and this is undocumented behavior and is not supported by GitHub. And while it works fine, I'm not sure we should be depending on behaviors that could be changed by GitHub any day without notice.

I'm wondering if we should just use standard HTTP conditional requests despite the drawback of having more cache misses when the credentials change

I'm wondering if we should just use standard HTTP conditional requests despite the drawback of having more cache misses when the credentials change

@alexymantha for github app it's not useful at all since we recreate the token on each reconciliation loop
for token since it's more static it could at least be useful

On our side we are using github app and we do understand the risk if Github change the Etag behaviour.
At least we need another flag for github app cache with a big warning about it

for github app it's not useful at all since we recreate the token on each reconciliation loop
for token

The way we've done it internally is to reuse the ghinstallation transport and it will only generate a new token when the old one expires, so the cache only has to rebuild every hour when the token changes.

It's a bit trickier here since the appset may not use the same credentials and may not have access to the same things so we would need to keep track of which transports to use, pretty similar to how you use the cache context to have different storage for different secrets

I will let the maintainers decide if they are confortable using the reverse-engineered behavior, I just wanted to highlight it.

• Security-wise, does the statement Using this reverse-engineered ETag algorithm, we can develop a [http.RoundTripper](https://pkg.go.dev/net/http#RoundTripper) that allows a GitHub REST API response that was cached/returned for a different Authorization header to be safely reused. mean we are risking AppSet to return data a user is not authorized to see?

I think it's ok, that's what I understood about this mechanism:

• Let's suppose the request was cached for an earlier Authorization header of a user.
• Now his Authorization header has changed
• ETAG is recalculated using the current Authorization header of the same user and put it into the http request
• Github identifies/authorizes the user according the Authorization header in the request and then checks the ETAG against the data and new Authorization header. If it matches it returns 304.

So it's always the same user

As discussed during the Argo CD Contributor Meeting 22th January with you @reggie-k

The unofficial features is not something that Argo CD will allow so the all PR need a revamp

Why this unofficial features was used? because of the way ArgoCD handle Github App token management
At every reconciliation loop, the ghinstallation.New is called that creates a new token to interact with Github API.
If the Github App transport is cached along with the token, the cache will be valid for the token life time (1h see)

As @crenshaw-dev mention the transport implementation need to follow the given principle:

as long as it’s cached at a granularity that doesn’t allow it to be used across security boundaries.

Move back the PR to draft and will rework the Github App token management and the cache to not use unofficial features.