joffrey-bion / krossbow

A Kotlin multiplatform coroutine-based STOMP client over websockets, with built-in conversions.
MIT License
189 stars 14 forks source link

Needed to add Sec-WebSocket-Protocol header to successfully make connection #492

Closed maxwellvogel closed 4 months ago

maxwellvogel commented 4 months ago

What happened?

I was unsuccessful in connecting and creating a stomp session from my iOS application. Adding a Sec-WebSocket-Protocol header to request allowed the handshake to complete successfully. My implementation in my common code is fairly simple, and worked without the header for my android application.

Reproduction and additional details

Common implementation:

class Messaging {
    private val httpClient = HttpClient {
        install(WebSockets)
        //adding header
        install(DefaultRequest){
            headers {
                append(HttpHeaders.SecWebSocketProtocol, "stomp")
            }
        }
    }
    private val ktorWebSocketClient = KtorWebSocketClient(httpClient)
    private val stompClient = StompClient(ktorWebSocketClient) {
        heartBeat = HeartBeat(10.seconds, 10.seconds)
        heartBeatTolerance = HeartBeatTolerance(Duration.ZERO, 10.seconds)
    }

    suspend fun connect(): Messaging {
        val session = stompClient.connect(STOMP_URL, STOMP_AUTHENTICATEDUSERNAME, STOMP_AUTHENTICATEDPASSWORD, customStompConnectHeaders = mapOf("Sec-WebSocket-Protocol" to "stomp")).withJsonConversions()
        return MessagingSession(session)
    }
}

class MessagingSession(private val stompSession: StompSessionWithKxSerialization) {
    suspend fun disconnect() = stompSession.disconnect()

    suspend fun watchForAnnouncerRunners(splitMappingId: Int): Flow<Runner> = stompSession.subscribe("/topic/splitMappingTimeUpdate_${splitMappingId}", Runner.serializer())
}

Here is some of the console for the warn and exception:

nw_ws_validate_server_response server response contains a Sec-WebSocket-Protocol value that was not advertised (stomp)

Task <4467FE75-8750-4EFC-AF44-94478838DE02>.<1> finished with error [-1011] Error Domain=NSURLErrorDomain Code=-1011 "There was a bad response from the server." UserInfo={NSErrorFailingURLStringKey=wss://removed:61619/stomp, NSErrorFailingURLKey=wss://removed:61619/stomp, _NSURLErrorWebSocketHandshakeFailureReasonKey=4, _NSURLErrorRelatedURLSessionTaskErrorKey=( "LocalWebSocketTask <4467FE75-8750-4EFC-AF44-94478838DE02>.<1>" ), _NSURLErrorFailingURLSessionTaskErrorKey=LocalWebSocketTask <4467FE75-8750-4EFC-AF44-94478838DE02>.<1>, NSLocalizedDescription=There was a bad response from the server.}

I can also note, that I had the same result with builtin as my client.

Krossbow version

6.0.0

Krossbow modules

krossbow-stomp-core, krossbow-stomp-kxserialization, krossbow-stomp-kxserialization-json, krossbow-websocket-core, krossbow-websocket-ktor

Kotlin version

2.0.0-RC1

Kotlin target platforms

Native - iOS

joffrey-bion commented 4 months ago

Hi, thanks a lot for the report! I'm sorry that you faced issues connecting to this server.

Note that the customStompConnectHeaders parameter (of StompClient.connect()) sets custom headers for the STOMP/CONNECT frame, which is a STOMP-level frame that is sent after the websocket handshake already happened. Setting headers through this parameter doesn't affect the websocket-level handshake. This means that only the Sec-WebSocket-Protocol header that you set on the default request in the Ktor client config has an effect on the handshake.

That being said, I'm surprised that you needed this header at all. I never had such a report before. Did you get the error server response contains a Sec-WebSocket-Protocol value that was not advertised (stomp) when you didn't send any additional headers? I'm guessing the iOS client might be more strict than the others in this respect. I will definitely need to add this header by default, with the supported STOMP protocol versions.

maxwellvogel commented 4 months ago

@joffrey-bion,

Yes, I got the server response contains a Sec-WebSocket-Protocol value that was not advertised (stomp) when I didn't send any headers.

Krossbow is awesome. I have had it bookmarked for a while, and finally got around to implementing it. Very slick.

joffrey-bion commented 4 months ago

Krossbow is awesome. I have had it bookmarked for a while, and finally got around to implementing it. Very slick.

Thank you so much, this is very appreciated 😊

And thanks for the additional information. I wonder if the server is supposed to send a protocol in the response if the client didn't negotiate any subprotocol in the handshake. What is the server you're using? Is it accessible publicly by any chance?

I will definitely add some STOMP subprotocol in the handshake, but it probably won't be stomp, but rather the more standard v12.stomp. I wonder if that will work if the server asks for stomp specifically.

joffrey-bion commented 4 months ago

@maxwellvogel I stand by what I said before and I still believe I should indeed make a change in Krossbow to add the subprotocol negotiation to the web socket handshake. That being said, after a little bit of research, I think the server you're using is broken and should be fixed for 2 reasons.

I double-checked the WS protocol specification section 4.2.2, and it stipulates the following regarding the Sec-WebSocket-Protocol header in the server response:

The value chosen MUST be derived from the client's handshake, specifically by selecting one of the values from the |Sec-WebSocket-Protocol| field that the server is willing to use for this connection (if any). If the client's handshake did not contain such a header field or if the server does not agree to any of the client's requested subprotocols, the only acceptable value is null. The absence of such a field is equivalent to the null value (meaning that if the server does not wish to agree to one of the suggested subprotocols, it MUST NOT send back a |Sec-WebSocket-Protocol| header field in its response).

Therefore, the fact that the server is responding with a Sec-WebSocket-Protocol header at all in the opening handshake response while no such header was sent by the client is a breach of the web socket protocol.

Also, I checked the IANA registry for web socket subprotocol names and stomp is not part of it. Only v10.stomp, v11.stomp, and v12.stomp are valid subprotocol names. It might be worth checking why the server is sending the plain stomp value, and change this to the proper STOMP subprotocol identifier.

Could you please confirm that using v12.stomp as header value from the client works in your case with that same server? I intend to make this the default, so I would like to double-check this.

Thanks a lot in advance!

maxwellvogel commented 4 months ago

@joffrey-bion

The host in my case is an Amazon MQ broker on AWS. It is configured as ActiveMQ 5.15.16. The STOMP endpoint is publically accessible, but would prefer to send you the endpoint privately.

I did confirm that "v12.stomp" works as the header value. The host responded with a Sec-Websocket-Protocol of "v12.stomp" when I sent a value of "v12.stomp". No clue why the default is just "stomp" when the request contains no value. I will note, my local ActiveMQ instance (v6.0.1) behaves the same way.

joffrey-bion commented 4 months ago

Thank you so much for the additional info!

If you want to share the STOMP endpoint privately, feel free to do so by email (public in my profile). But you don't have to if you don't feel comfortable with this, I think I have enough to go by for now thanks to your experiment with the header value.

joffrey-bion commented 4 months ago

Closing this as superseded by the more general subprotocol negotiation feature: https://github.com/joffrey-bion/krossbow/issues/497