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

[extension/auth] Support authentication for websockets #10864

Open
BinaryFissionGames opened this issue Aug 13, 2024 · 4 comments
Open

[extension/auth] Support authentication for websockets #10864

BinaryFissionGames opened this issue Aug 13, 2024 · 4 comments
Assignees
Labels
auth Authentication related

Comments

@BinaryFissionGames
Copy link

BinaryFissionGames commented Aug 13, 2024

Is your feature request related to a problem? Please describe.
Currently, the authentication extension client interface supports HTTP and gRPC authentication. However, we would like to be able to authenticate websocket connections as well, in particular for supporting authentication for the opamp extension, which can be run using websockets.

On the surface, it seems like the HTTP RoundTripper should be applicable to websocket connections, but the most popular libraries do not take a RoundTripper.

See: github.com/gorilla/websocket, golang.org/x/net/websocket

Instead, they are limited essentially to a URL + Headers.

Describe the solution you'd like
Here are a couple ways we could support authentication for websockets:

1. Try using the existing RoundTripper

This is a straight-forward "hacky" way to accomplish authentication. Basically the idea would be to implement a "base" round-tripper that modifies a struct to pass the websocket connection transparently through the "wrapper" round-tripper, for example:

type baseRoundTripper struct {
	dialer websocket.Dialer
	conn   *websocket.Conn
}

func (b baseRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) {
	conn, resp, err := b.dialer.Dial(req.URL.String(), req.Header)
	if err != nil {
		return resp, err
	}

	b.conn = conn
	return resp, err
}]

Here, the connection is preserved on the base RoundTripper and could be retrieved after

The issue with this is that it assumes a few things about the function:

  1. the base RoundTripper must only succeed once (since a connection is long-lived, and does not actually do a single "round-trip") - calling a second time after failure is OK
  2. the base RoundTripper must only modify the URL or Headers. If anything else of the request is modified (e.g. the body), it will be ignored.

Because of these limitations, it's possible for an authentication extension to follow the interface, but be incompatible with authenticating over a websocket. That feels undesirable to me, but I'm leaving it here as an option; It could be so uncommon for anything other than the headers to be modified that it's worth doing this more hacky solution. (See appendix, "Calls RoundTripper Once", "Modifies Other")

2. Add a method that just gets request headers

type Client interface {
	WebsocketHeaders() (http.Headers, error)
}

Here, you could imagine fetching headers for the request before making it. This approach pretty simplistic, but seems like it could work for most authentication cases (See appendix, "Uses HTTP response").

The downside here is that nothing can happen after the request, because the extension doesn't have a hook to get the http response. It seems like most of the current authentication extensions don't make use of this and always call the RoundTripper last.

3. Add a restricted "url + headers" method

You could imagine an interface like this (ignore the clumsy naming):

type HeaderRequester interface {
	DoRequestWithHeaders(url string, headers http.Header) http.Response	
}

type Client interface {
	HeaderRequester(base HeaderRequester) HeaderRequester
}

Essentially, this is the same thing as the http.RoundTripper, but the surface area of the request is reduced to just the URL + headers, which is fully compatible with the interface for dialing a websocket in all the most popular websocket libraries. In order to make this work with websockets, we'd still have the downside of only allowing the base requester to be called once.

4. Add a method for dialing a websocket directly

We could decide on directly supporting dialing for a particular websocket library. For instance:

type WebsocketDialer interface {
		DialWebsocket(url string, headers http.Header) (*websocket.Conn, *http.Response, error)
}

type Client interface {
	WebsocketDialer(base WebsocketDialer) (WebsocketDialer, error)
}

The huge downside here is that this creates a dependency on a particular library (in this case, the gorilla library). It's not unreasonable to assume that component may want to use a different library, or even that at some point we may need to switch libraries. I think that makes this option not suitable, but it's worth mentioning if we want to tie ourselves to a particular library, we get a "simpler" interface here.

5. Create a Dial interface that returns a Conn interface

Like 4, we essentially have a dial method, but instead of using concrete types we could return an interface like this:

type MessageType int

const (
// These values align with RFC6455 opcodes https://www.rfc-editor.org/rfc/rfc6455.html#section-11.8
TextMessage MessageType = 1
BinaryMessage MessageType = 2
)

type WebsocketConn interface {
	WriteMessage(ctx context.Context, msgType MessageType, msg []byte) error
	ReadMesage(ctx context.Context) (MessageType, []byte, error)
	Close(code int, reason string) error
}

type Dialer interface {
	Dial(url string, headers http.Header) (WebsocketConn, http.Response, error)
}

type Client interface {
	WebsocketDialer(base websocket.Dialer) (websocket.Dialer, error)
}

The con here is that we have to essentially define our own interface for a websocket connection. It's also on the user of this interface to create an adapter for handling websocket connections.

Appendix: Table of client auth extensions and RoundTripper use

Note: The vendor specific ones here likely have no use to a generic websocket-based component (like the opamp extension), since they are for specific cloud platforms. I've left them in the table for consideration of how different extensions using RoundTripper might be implemented.

Calls RoundTripper Once Modifies Headers Modifies URL Modifies other Uses HTTP response Vendor specific
basicauthextension + + - - - -
bearertokenauthextension + + - - - -
headerssetterextension + + - - - -
oauth2clientauthextension + + - - - -
asapauthextension + + - - - +
googleclientauthextension + + - - - +
sigv4authextension + + + - (note: reads body) - +
sumologicextension + + (note: Modifies cookies) - - + (uses response to update sticky session cookie) +

Additional context
See #32762 which is tracking using existing authentication extensions with the supervisor.

@jpkrohling jpkrohling added the auth Authentication related label Aug 27, 2024
@jpkrohling jpkrohling self-assigned this Aug 27, 2024
@jpkrohling
Copy link
Member

I see websockets in the same position as gRPC, in that a per-RPC auth call is desirable. In that sense, I'd go with option 2, which maps nicely with the pattern we have for gRPC as well.

That said, as we are trying to stabilize everything., I'd prefer this change to be made after v1 of this module.

@BinaryFissionGames
Copy link
Author

That makes sense to me.

@djaglowski
Copy link
Member

Was this completed by open-telemetry/opentelemetry-collector-contrib#35507?

@BinaryFissionGames
Copy link
Author

BinaryFissionGames commented Dec 13, 2024

open-telemetry/opentelemetry-collector-contrib#35507 was for a hacky fix that can fail depending on the extension implementation. Ideally this would be fully supported by the core repo.

Ideally, we'd implement method 2 here. The current implementation is method 1, which is error prone and will totally cause issues if you use certain auth extensions (e.g. sigv4auth extensions). Ideally, extensions would be able to somehow declare whether they are compatible with websockets or not (e.g. by implementing one of the proposed interfaces).

Related issue, gorilla/websocket#959 - this would make the first option fully compatible, but I could also see there being edgecases that cause issues still even if that did get implemented.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auth Authentication related
Projects
None yet
Development

No branches or pull requests

3 participants