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

Authentication Support #5

Closed
t-chaik opened this issue Mar 11, 2019 · 7 comments
Closed

Authentication Support #5

t-chaik opened this issue Mar 11, 2019 · 7 comments

Comments

@t-chaik
Copy link

t-chaik commented Mar 11, 2019

Continuation of buildbarn/bb-storage#2: bb-storage is gaining authentication support and in order for that to be useful, bb-browser also should. That would require supporting TLS at the web endpoint and credential forwarding to backend storage when possible I believe.

@EdSchouten
Copy link
Member

EdSchouten commented Mar 11, 2019

Hi Martin,

We don't do any client certificate validation on the WWW endpoint, right? The only thing we want to ensure is that people use TLS when connecting to the service, so that JWTs never go over the line in unencrypted form.

I'd rather see that logic for enforcing TLS, doing HTTP redirects to a login service to request tokens, etc. etc. is handled through separate services. For example, when running web services on Kubernetes clusters, it's pretty common to use ingress controllers to enforce exactly that. Envoy is a commonly used tool for doing that kind of stuff.

@t-chaik
Copy link
Author

t-chaik commented Mar 12, 2019

We don't do any client certificate validation on the WWW endpoint, right?

Sure, TLS client authentication would be pointless for such a web service, especially considering that, as you pointed out, the service would probably be deploy behind a proxy anyway.

I'd rather see that logic for enforcing TLS, doing HTTP redirects to a login service to request tokens, etc. etc. is handled through separate services.

Agreed, there is no point in reimplementing all this in bb-browser itself, I think the only thing that would be great to have is token forwarding in order to access remote CAS data (that means the authorization header has to reach the service though). If we don't want that, then we would need static credentials in blobstore configuration in order to access remote CAS servers I believe. What do you think?

@EdSchouten
Copy link
Member

EdSchouten commented Mar 14, 2019

I think the only thing that would be great to have is token forwarding in order to access remote CAS data (that means the authorization header has to reach the service though).

Exactly. The only thing we have to be careful of in the long run is that if bb-browser were to be configured to also do local caching of blobs, accessing a cached blob would allow you to bypass authentication. We'd need to add some logic to bb-browser to deny accessing cached data in case of the first visit. That said, irrelevant at this point in time.

@t-chaik
Copy link
Author

t-chaik commented Mar 15, 2019

I've just submitted #6 for server-side TLS support in bb-browser.

HTTP credential forwarding will have to be implemented in bb-storage I believe, I'm thinking of introducing an "http_metadata_forwarding" adaptor.

@EdSchouten
Copy link
Member

HTTP credential forwarding will have to be implemented in bb-storage I believe, I'm thinking of introducing an "http_metadata_forwarding" adaptor.

This requires us to access headers in the HTTP request, right? BlobAccess would not have any access to that, I suppose.

The easiest thing to do right now would be to add some utility function to BrowserService to wrap access to req.Context() to add gRPC client credentials to it. This doesn't really need to be done in a tidy way, because when #4 is tackled, it might be the case that we can entirely rely on gRPC-Web to do the credential forwarding for us.

@t-chaik
Copy link
Author

t-chaik commented Mar 18, 2019

This requires us to access headers in the HTTP request, right? BlobAccess would not have any access to that, I suppose.

Indeed, BlobAccess only has access to the context, which is taken from req.Context() in that case. I was hoping it would be enough but req.Context() does not contain any auth. metadata, so I guess you are right, we'll have to feed the auth. metadata into the context from the request headers before delegating the call to BlobAccess.

@t-chaik
Copy link
Author

t-chaik commented Mar 19, 2019

#7 has landed, thanks to @EdSchouten; closing.

@t-chaik t-chaik closed this as completed Mar 19, 2019
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

No branches or pull requests

2 participants