From cd652dca756ce7748ad0ba0dc4270110fb11b198 Mon Sep 17 00:00:00 2001 From: Rob Ede Date: Sun, 28 Feb 2021 19:55:34 +0000 Subject: [PATCH] refactor websocket key hashing (#2035) --- actix-http/CHANGES.md | 2 + actix-http/Cargo.toml | 4 ++ actix-http/examples/ws.rs | 107 +++++++++++++++++++++++++++++ actix-http/src/ws/mod.rs | 27 +++++--- actix-http/src/ws/proto.rs | 134 +++++++++++++++++++++---------------- actix-web-actors/src/ws.rs | 11 ++- awc/src/error.rs | 17 +++-- awc/src/ws.rs | 8 ++- 8 files changed, 234 insertions(+), 76 deletions(-) create mode 100644 actix-http/examples/ws.rs diff --git a/actix-http/CHANGES.md b/actix-http/CHANGES.md index 6ba111eb3..165b004a6 100644 --- a/actix-http/CHANGES.md +++ b/actix-http/CHANGES.md @@ -3,6 +3,7 @@ ## Unreleased - 2021-xx-xx ### Changed * Feature `cookies` is now optional and disabled by default. [#1981] +* `ws::hash_key` now returns array. [#2035] ### Removed * re-export of `futures_channel::oneshot::Canceled` is removed from `error` mod. [#1994] @@ -10,6 +11,7 @@ [#1981]: https://github.com/actix/actix-web/pull/1981 [#1994]: https://github.com/actix/actix-web/pull/1994 +[#2035]: https://github.com/actix/actix-web/pull/2035 ## 3.0.0-beta.3 - 2021-02-10 diff --git a/actix-http/Cargo.toml b/actix-http/Cargo.toml index 96967e18a..d7915ddf9 100644 --- a/actix-http/Cargo.toml +++ b/actix-http/Cargo.toml @@ -103,6 +103,10 @@ version = "0.10.9" package = "openssl" features = ["vendored"] +[[example]] +name = "ws" +required-features = ["rustls"] + [[bench]] name = "write-camel-case" harness = false diff --git a/actix-http/examples/ws.rs b/actix-http/examples/ws.rs new file mode 100644 index 000000000..4e03aa8ab --- /dev/null +++ b/actix-http/examples/ws.rs @@ -0,0 +1,107 @@ +//! Sets up a WebSocket server over TCP and TLS. +//! Sends a heartbeat message every 4 seconds but does not respond to any incoming frames. + +extern crate tls_rustls as rustls; + +use std::{ + env, io, + pin::Pin, + task::{Context, Poll}, + time::Duration, +}; + +use actix_codec::Encoder; +use actix_http::{error::Error, ws, HttpService, Request, Response}; +use actix_rt::time::{interval, Interval}; +use actix_server::Server; +use bytes::{Bytes, BytesMut}; +use bytestring::ByteString; +use futures_core::{ready, Stream}; + +#[actix_rt::main] +async fn main() -> io::Result<()> { + env::set_var("RUST_LOG", "actix=info,h2_ws=info"); + env_logger::init(); + + Server::build() + .bind("tcp", ("127.0.0.1", 8080), || { + HttpService::build().h1(handler).tcp() + })? + .bind("tls", ("127.0.0.1", 8443), || { + HttpService::build().finish(handler).rustls(tls_config()) + })? + .run() + .await +} + +async fn handler(req: Request) -> Result { + log::info!("handshaking"); + let mut res = ws::handshake(req.head())?; + + // handshake will always fail under HTTP/2 + + log::info!("responding"); + Ok(res.streaming(Heartbeat::new(ws::Codec::new()))) +} + +struct Heartbeat { + codec: ws::Codec, + interval: Interval, +} + +impl Heartbeat { + fn new(codec: ws::Codec) -> Self { + Self { + codec, + interval: interval(Duration::from_secs(4)), + } + } +} + +impl Stream for Heartbeat { + type Item = Result; + + fn poll_next( + mut self: Pin<&mut Self>, + cx: &mut Context<'_>, + ) -> Poll> { + log::trace!("poll"); + + ready!(self.as_mut().interval.poll_tick(cx)); + + let mut buffer = BytesMut::new(); + + self.as_mut() + .codec + .encode( + ws::Message::Text(ByteString::from_static("hello world")), + &mut buffer, + ) + .unwrap(); + + Poll::Ready(Some(Ok(buffer.freeze()))) + } +} + +fn tls_config() -> rustls::ServerConfig { + use std::io::BufReader; + + use rustls::{ + internal::pemfile::{certs, pkcs8_private_keys}, + NoClientAuth, ServerConfig, + }; + + let cert = rcgen::generate_simple_self_signed(vec!["localhost".to_owned()]).unwrap(); + let cert_file = cert.serialize_pem().unwrap(); + let key_file = cert.serialize_private_key_pem(); + + let mut config = ServerConfig::new(NoClientAuth::new()); + let cert_file = &mut BufReader::new(cert_file.as_bytes()); + let key_file = &mut BufReader::new(key_file.as_bytes()); + + let cert_chain = certs(cert_file).unwrap(); + let mut keys = pkcs8_private_keys(key_file).unwrap(); + config.set_single_cert(cert_chain, keys.remove(0)).unwrap(); + + config +} diff --git a/actix-http/src/ws/mod.rs b/actix-http/src/ws/mod.rs index 0490163d5..cec73db96 100644 --- a/actix-http/src/ws/mod.rs +++ b/actix-http/src/ws/mod.rs @@ -1,4 +1,4 @@ -//! WebSocket protocol. +//! WebSocket protocol implementation. //! //! To setup a WebSocket, first perform the WebSocket handshake then on success convert `Payload` into a //! `WsStream` stream and then use `WsWriter` to communicate with the peer. @@ -8,9 +8,12 @@ use std::io; use derive_more::{Display, Error, From}; use http::{header, Method, StatusCode}; -use crate::error::ResponseError; -use crate::message::RequestHead; -use crate::response::{Response, ResponseBuilder}; +use crate::{ + error::ResponseError, + header::HeaderValue, + message::RequestHead, + response::{Response, ResponseBuilder}, +}; mod codec; mod dispatcher; @@ -89,7 +92,7 @@ pub enum HandshakeError { NoVersionHeader, /// Unsupported WebSocket version. - #[display(fmt = "Unsupported version.")] + #[display(fmt = "Unsupported WebSocket version.")] UnsupportedVersion, /// WebSocket key is not set or wrong. @@ -105,19 +108,19 @@ impl ResponseError for HandshakeError { .finish(), HandshakeError::NoWebsocketUpgrade => Response::BadRequest() - .reason("No WebSocket UPGRADE header found") + .reason("No WebSocket Upgrade header found") .finish(), HandshakeError::NoConnectionUpgrade => Response::BadRequest() - .reason("No CONNECTION upgrade") + .reason("No Connection upgrade") .finish(), HandshakeError::NoVersionHeader => Response::BadRequest() - .reason("Websocket version header is required") + .reason("WebSocket version header is required") .finish(), HandshakeError::UnsupportedVersion => Response::BadRequest() - .reason("Unsupported version") + .reason("Unsupported WebSocket version") .finish(), HandshakeError::BadWebsocketKey => { @@ -193,7 +196,11 @@ pub fn handshake_response(req: &RequestHead) -> ResponseBuilder { Response::build(StatusCode::SWITCHING_PROTOCOLS) .upgrade("websocket") .insert_header((header::TRANSFER_ENCODING, "chunked")) - .insert_header((header::SEC_WEBSOCKET_ACCEPT, key)) + .insert_header(( + header::SEC_WEBSOCKET_ACCEPT, + // key is known to be header value safe ascii + HeaderValue::from_bytes(&key).unwrap(), + )) .take() } diff --git a/actix-http/src/ws/proto.rs b/actix-http/src/ws/proto.rs index 1e8bf7af3..fdcde5eac 100644 --- a/actix-http/src/ws/proto.rs +++ b/actix-http/src/ws/proto.rs @@ -1,5 +1,7 @@ -use std::convert::{From, Into}; -use std::fmt; +use std::{ + convert::{From, Into}, + fmt, +}; /// Operation codes as part of RFC6455. #[derive(Debug, Eq, PartialEq, Clone, Copy)] @@ -28,8 +30,9 @@ pub enum OpCode { impl fmt::Display for OpCode { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - use self::OpCode::*; - match *self { + use OpCode::*; + + match self { Continue => write!(f, "CONTINUE"), Text => write!(f, "TEXT"), Binary => write!(f, "BINARY"), @@ -44,6 +47,7 @@ impl fmt::Display for OpCode { impl From for u8 { fn from(op: OpCode) -> u8 { use self::OpCode::*; + match op { Continue => 0, Text => 1, @@ -62,6 +66,7 @@ impl From for u8 { impl From for OpCode { fn from(byte: u8) -> OpCode { use self::OpCode::*; + match byte { 0 => Continue, 1 => Text, @@ -77,63 +82,66 @@ impl From for OpCode { /// Status code used to indicate why an endpoint is closing the WebSocket connection. #[derive(Debug, Eq, PartialEq, Clone, Copy)] pub enum CloseCode { - /// Indicates a normal closure, meaning that the purpose for - /// which the connection was established has been fulfilled. + /// Indicates a normal closure, meaning that the purpose for which the connection was + /// established has been fulfilled. Normal, - /// Indicates that an endpoint is "going away", such as a server - /// going down or a browser having navigated away from a page. + + /// Indicates that an endpoint is "going away", such as a server going down or a browser having + /// navigated away from a page. Away, - /// Indicates that an endpoint is terminating the connection due - /// to a protocol error. + + /// Indicates that an endpoint is terminating the connection due to a protocol error. Protocol, - /// Indicates that an endpoint is terminating the connection - /// because it has received a type of data it cannot accept (e.g., an - /// endpoint that understands only text data MAY send this if it + + /// Indicates that an endpoint is terminating the connection because it has received a type of + /// data it cannot accept (e.g., an endpoint that understands only text data MAY send this if it /// receives a binary message). Unsupported, - /// Indicates an abnormal closure. If the abnormal closure was due to an - /// error, this close code will not be used. Instead, the `on_error` method - /// of the handler will be called with the error. However, if the connection - /// is simply dropped, without an error, this close code will be sent to the - /// handler. + + /// Indicates an abnormal closure. If the abnormal closure was due to an error, this close code + /// will not be used. Instead, the `on_error` method of the handler will be called with + /// the error. However, if the connection is simply dropped, without an error, this close code + /// will be sent to the handler. Abnormal, - /// Indicates that an endpoint is terminating the connection - /// because it has received data within a message that was not - /// consistent with the type of the message (e.g., non-UTF-8 \[RFC3629\] + + /// Indicates that an endpoint is terminating the connection because it has received data within + /// a message that was not consistent with the type of the message (e.g., non-UTF-8 \[RFC3629\] /// data within a text message). Invalid, - /// Indicates that an endpoint is terminating the connection - /// because it has received a message that violates its policy. This - /// is a generic status code that can be returned when there is no - /// other more suitable status code (e.g., Unsupported or Size) or if there - /// is a need to hide specific details about the policy. + + /// Indicates that an endpoint is terminating the connection because it has received a message + /// that violates its policy. This is a generic status code that can be returned when there is + /// no other more suitable status code (e.g., Unsupported or Size) or if there is a need to hide + /// specific details about the policy. Policy, - /// Indicates that an endpoint is terminating the connection - /// because it has received a message that is too big for it to - /// process. + + /// Indicates that an endpoint is terminating the connection because it has received a message + /// that is too big for it to process. Size, - /// Indicates that an endpoint (client) is terminating the - /// connection because it has expected the server to negotiate one or - /// more extension, but the server didn't return them in the response - /// message of the WebSocket handshake. The list of extensions that - /// are needed should be given as the reason for closing. - /// Note that this status code is not used by the server, because it - /// can fail the WebSocket handshake instead. + + /// Indicates that an endpoint (client) is terminating the connection because it has expected + /// the server to negotiate one or more extension, but the server didn't return them in the + /// response message of the WebSocket handshake. The list of extensions that are needed should + /// be given as the reason for closing. Note that this status code is not used by the server, + /// because it can fail the WebSocket handshake instead. Extension, - /// Indicates that a server is terminating the connection because - /// it encountered an unexpected condition that prevented it from - /// fulfilling the request. + + /// Indicates that a server is terminating the connection because it encountered an unexpected + /// condition that prevented it from fulfilling the request. Error, - /// Indicates that the server is restarting. A client may choose to - /// reconnect, and if it does, it should use a randomized delay of 5-30 - /// seconds between attempts. + + /// Indicates that the server is restarting. A client may choose to reconnect, and if it does, + /// it should use a randomized delay of 5-30 seconds between attempts. Restart, - /// Indicates that the server is overloaded and the client should either - /// connect to a different IP (when multiple targets exist), or - /// reconnect to the same IP when a user has performed an action. + + /// Indicates that the server is overloaded and the client should either connect to a different + /// IP (when multiple targets exist), or reconnect to the same IP when a user has performed + /// an action. Again, + #[doc(hidden)] Tls, + #[doc(hidden)] Other(u16), } @@ -141,6 +149,7 @@ pub enum CloseCode { impl From for u16 { fn from(code: CloseCode) -> u16 { use self::CloseCode::*; + match code { Normal => 1000, Away => 1001, @@ -163,6 +172,7 @@ impl From for u16 { impl From for CloseCode { fn from(code: u16) -> CloseCode { use self::CloseCode::*; + match code { 1000 => Normal, 1001 => Away, @@ -210,17 +220,29 @@ impl> From<(CloseCode, T)> for CloseReason { } } -static WS_GUID: &str = "258EAFA5-E914-47DA-95CA-C5AB0DC85B11"; +/// The WebSocket GUID as stated in the spec. See https://tools.ietf.org/html/rfc6455#section-1.3. +static WS_GUID: &[u8] = b"258EAFA5-E914-47DA-95CA-C5AB0DC85B11"; -// TODO: hash is always same size, we don't need String -pub fn hash_key(key: &[u8]) -> String { - use sha1::Digest; - let mut hasher = sha1::Sha1::new(); +/// Hashes the `Sec-WebSocket-Key` header according to the WebSocket spec. +/// +/// Result is a Base64 encoded byte array. `base64(sha1(input))` is always 28 bytes. +pub fn hash_key(key: &[u8]) -> [u8; 28] { + let hash = { + use sha1::Digest as _; - hasher.update(key); - hasher.update(WS_GUID.as_bytes()); + let mut hasher = sha1::Sha1::new(); - base64::encode(&hasher.finalize()) + hasher.update(key); + hasher.update(WS_GUID); + + hasher.finalize() + }; + + let mut hash_b64 = [0; 28]; + let n = base64::encode_config_slice(&hash, base64::STANDARD, &mut hash_b64); + assert_eq!(n, 28); + + hash_b64 } #[cfg(test)] @@ -288,11 +310,11 @@ mod test { #[test] fn test_hash_key() { let hash = hash_key(b"hello actix-web"); - assert_eq!(&hash, "cR1dlyUUJKp0s/Bel25u5TgvC3E="); + assert_eq!(&hash, b"cR1dlyUUJKp0s/Bel25u5TgvC3E="); } #[test] - fn closecode_from_u16() { + fn close_code_from_u16() { assert_eq!(CloseCode::from(1000u16), CloseCode::Normal); assert_eq!(CloseCode::from(1001u16), CloseCode::Away); assert_eq!(CloseCode::from(1002u16), CloseCode::Protocol); @@ -310,7 +332,7 @@ mod test { } #[test] - fn closecode_into_u16() { + fn close_code_into_u16() { assert_eq!(1000u16, Into::::into(CloseCode::Normal)); assert_eq!(1001u16, Into::::into(CloseCode::Away)); assert_eq!(1002u16, Into::::into(CloseCode::Protocol)); diff --git a/actix-web-actors/src/ws.rs b/actix-web-actors/src/ws.rs index 1ab4cfce5..de2802d21 100644 --- a/actix-web-actors/src/ws.rs +++ b/actix-web-actors/src/ws.rs @@ -15,10 +15,13 @@ use actix::{ SpawnHandle, }; use actix_codec::{Decoder, Encoder}; -use actix_http::ws::{hash_key, Codec}; pub use actix_http::ws::{ CloseCode, CloseReason, Frame, HandshakeError, Message, ProtocolError, }; +use actix_http::{ + http::HeaderValue, + ws::{hash_key, Codec}, +}; use actix_web::dev::HttpResponseBuilder; use actix_web::error::{Error, PayloadError}; use actix_web::http::{header, Method, StatusCode}; @@ -162,7 +165,11 @@ pub fn handshake_with_protocols( let mut response = HttpResponse::build(StatusCode::SWITCHING_PROTOCOLS) .upgrade("websocket") - .insert_header((header::SEC_WEBSOCKET_ACCEPT, key)) + .insert_header(( + header::SEC_WEBSOCKET_ACCEPT, + // key is known to be header value safe ascii + HeaderValue::from_bytes(&key).unwrap(), + )) .take(); if let Some(protocol) = protocol { diff --git a/awc/src/error.rs b/awc/src/error.rs index f86224e62..b715f6213 100644 --- a/awc/src/error.rs +++ b/awc/src/error.rs @@ -18,24 +18,31 @@ pub enum WsClientError { /// Invalid response status #[display(fmt = "Invalid response status")] InvalidResponseStatus(StatusCode), + /// Invalid upgrade header #[display(fmt = "Invalid upgrade header")] InvalidUpgradeHeader, + /// Invalid connection header #[display(fmt = "Invalid connection header")] InvalidConnectionHeader(HeaderValue), - /// Missing CONNECTION header - #[display(fmt = "Missing CONNECTION header")] + + /// Missing Connection header + #[display(fmt = "Missing Connection header")] MissingConnectionHeader, - /// Missing SEC-WEBSOCKET-ACCEPT header - #[display(fmt = "Missing SEC-WEBSOCKET-ACCEPT header")] + + /// Missing Sec-Websocket-Accept header + #[display(fmt = "Missing Sec-Websocket-Accept header")] MissingWebSocketAcceptHeader, + /// Invalid challenge response #[display(fmt = "Invalid challenge response")] - InvalidChallengeResponse(String, HeaderValue), + InvalidChallengeResponse([u8; 28], HeaderValue), + /// Protocol error #[display(fmt = "{}", _0)] Protocol(WsProtocolError), + /// Send request error #[display(fmt = "{}", _0)] SendRequest(SendRequestError), diff --git a/awc/src/ws.rs b/awc/src/ws.rs index 5f4570963..1aa426ac7 100644 --- a/awc/src/ws.rs +++ b/awc/src/ws.rs @@ -381,12 +381,14 @@ impl WebsocketsRequest { if let Some(hdr_key) = head.headers.get(&header::SEC_WEBSOCKET_ACCEPT) { let encoded = ws::hash_key(key.as_ref()); - if hdr_key.as_bytes() != encoded.as_bytes() { + + if hdr_key.as_bytes() != &encoded { log::trace!( - "Invalid challenge response: expected: {} received: {:?}", - encoded, + "Invalid challenge response: expected: {:?} received: {:?}", + &encoded, key ); + return Err(WsClientError::InvalidChallengeResponse( encoded, hdr_key.clone(),