Conversation
| } | ||
|
|
||
| // Close closes the authenticator. | ||
| func (a *Authenticator) Close(_ context.Context) error { |
There was a problem hiding this comment.
nit: should we clear the token here?
If we consider it sensitive data, even though it will be garbage collected, we might want to clear it directly.
a.token = ""
return nil
| var _ commandrunner.Hook = (*Authenticator)(nil) | ||
|
|
||
| // NewAuthenticator creates a new Authenticator | ||
| func NewAuthenticator(attuneEndpoint, token string) (*Authenticator, error) { |
There was a problem hiding this comment.
my usual nit 😄 thinking would not hurt to add a guard clause
if token == "" {
return nil, fmt.Errorf("token must not be nil")
}
aadc-dev
left a comment
aadc-dev
left a comment
There was a problem hiding this comment.
LGTM, just some comments, non blocking
|
|
||
| // Writers | ||
|
|
||
| type contextWriter struct { |
There was a problem hiding this comment.
I think the code could be simplified a bit removing the contextWriter. The contextReader will respond to the context cancellation and end the read-write loop in io.Copy already. It also adds some timing dependent branching that could make the logic a bit inconsistent. Depending on when the context is cancelled, it will sometimes fail on read or sometimes on write (which means sometimes we don't write data we read).
I don't think there's any meaningful benefits to also implementing the same logic for the write portion. It's not a big deal either way but it does mean we have ~100LOC we don't have to maintain.
| // Copy is a contextual version of [io.Copy]. If the context is cancelled, calls to | ||
| // read and write functions will error, stopping the copy. | ||
| // This retains all properties of [io.Copy], including support for [io.WriterTo] and | ||
| // [io.ReaderFrom]. |
There was a problem hiding this comment.
This got me thinking. If we support io.WriterTo and io.ReaderFrom, we cannot guarantee that the context cancellation will actually propagate. Is there a need to actually support them? If so I think that just closing the files on context cancellation would be a bit of an easier solution that handles these cases. It does come with the downside that closing the files could result in inconsistent errors though.
| if host, port, err := net.SplitHostPort(attuneEndpoint); err == nil { | ||
| a.attuneEndpointHost = host | ||
| a.attuneEndpointPort = port | ||
| return nil | ||
| } | ||
|
|
There was a problem hiding this comment.
nit: Not sure if this function is meant to be exhaustive but I did notice that unhandled schemes could fall-through to here and it would return a nil error.
a := &Authenticator{}
// i.e. with a typo
if err := a.setHostPort("httpss://attune-endpoint.sh"); err != nil {
log.Fatal(err) // skipped: above will return nil
}
fmt.Println(a.attuneEndpointHost == "httpss") // true
fmt.Println(a.attuneEndpointPort == "//attune-endpoint.sh") // trueIt will get caught anyway during the setup function when it tries to startup the server but that may end up being a red herring if caught in logs. Might just be better to handle the error here.
| // It does this by creating a local TCP proxy that wraps the connection in TLS, forwarding | ||
| // it to a reverse proxy in front of Attune. The reverse proxy handles authentication, and | ||
| // strips the TLS layer. | ||
| type Authenticator struct { |
There was a problem hiding this comment.
I believe I understand why we need this Authenticator but I'm not entirely sure. A little bit more context in the commentary would be helpful.
From what I can imagine, this is necessary because Attune does not support mTLS natively? Or we have certain implementation requirements for it that Attune does not meet? In that case this implements a custom authentication middleware between the Attune client and control plane.
3 participants
This adds support for token and mTLS authentication methods for Attune to OPRT2 based on #400.
mTLS is more or less a re-implementation of what Teleport does for app access, except it shouldn't be affected by the same performance issues that occur with Teleport and large file transfers/large HTTP requests.
Goal issue: https://github.com/gravitational/cloud/issues/15375