refactor websocket key hashing (#2035)

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

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

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<Response, Error> {
+    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<Bytes, Error>;
+
+    fn poll_next(
+        mut self: Pin<&mut Self>,
+        cx: &mut Context<'_>,
+    ) -> Poll<Option<Self::Item>> {
+        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
+}

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::{
-use crate::message::RequestHead;
+    error::ResponseError,
-use crate::response::{Response, ResponseBuilder};
+    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()
 }
 

actix-http/src/ws/proto.rs

@@ -1,5 +1,7 @@
-use std::convert::{From, Into};
+use std::{
-use std::fmt;
+    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::*;
+        use OpCode::*;
-        match *self {
+
+        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<OpCode> for u8 {
     fn from(op: OpCode) -> u8 {
         use self::OpCode::*;
+
         match op {
             Continue => 0,
             Text => 1,
@@ -62,6 +66,7 @@ impl From<OpCode> for u8 {
 impl From<u8> for OpCode {
     fn from(byte: u8) -> OpCode {
         use self::OpCode::*;
+
         match byte {
             0 => Continue,
             1 => Text,
@@ -77,63 +82,66 @@ impl From<u8> 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
+    /// Indicates a normal closure, meaning that the purpose for which the connection was
-    /// which the connection was established has been fulfilled.
+    /// 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
+    /// Indicates that an endpoint is terminating the connection because it has received a type of
-    /// endpoint that understands only text data MAY send this if it
+    /// 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
+    /// Indicates an abnormal closure. If the abnormal closure was due to an error, this close code
-    /// of the handler will be called with the error. However, if the connection
+    /// will not be used. Instead, the `on_error` method of the handler will be called with
-    /// is simply dropped, without an error, this close code will be sent to the
+    /// the error. However, if the connection is simply dropped, without an error, this close code
-    /// handler.
+    /// 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
+    /// Indicates that an endpoint is terminating the connection because it has received data within
-    /// consistent with the type of the message (e.g., non-UTF-8 \[RFC3629\]
+    /// 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
+    /// Indicates that an endpoint is terminating the connection because it has received a message
-    /// is a generic status code that can be returned when there is no
+    /// that violates its policy. This is a generic status code that can be returned when there is
-    /// other more suitable status code (e.g., Unsupported or Size) or if there
+    /// no other more suitable status code (e.g., Unsupported or Size) or if there is a need to hide
-    /// is a need to hide specific details about the policy.
+    /// 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
+    /// Indicates that an endpoint is terminating the connection because it has received a message
-    /// process.
+    /// 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
+    /// Indicates that an endpoint (client) is terminating the connection because it has expected
-    /// more extension, but the server didn't return them in the response
+    /// the server to negotiate one or more extension, but the server didn't return them in the
-    /// message of the WebSocket handshake.  The list of extensions that
+    /// response message of the WebSocket handshake.  The list of extensions that are needed should
-    /// are needed should be given as the reason for closing.
+    /// be given as the reason for closing. Note that this status code is not used by the server,
-    /// Note that this status code is not used by the server, because it
+    /// because it can fail the WebSocket handshake instead.
-    /// 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
+    /// Indicates that a server is terminating the connection because it encountered an unexpected
-    /// fulfilling the request.
+    /// 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
+    /// Indicates that the server is restarting. A client may choose to reconnect, and if it does,
-    /// seconds between attempts.
+    /// 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
+    /// Indicates that the server is overloaded and the client should either connect to a different
-    /// reconnect to the same IP when a user has performed an action.
+    /// 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<CloseCode> for u16 {
     fn from(code: CloseCode) -> u16 {
         use self::CloseCode::*;
+
         match code {
             Normal => 1000,
             Away => 1001,
@@ -163,6 +172,7 @@ impl From<CloseCode> for u16 {
 impl From<u16> for CloseCode {
     fn from(code: u16) -> CloseCode {
         use self::CloseCode::*;
+
         match code {
             1000 => Normal,
             1001 => Away,
@@ -210,17 +220,29 @@ impl<T: Into<String>> 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
+/// Hashes the `Sec-WebSocket-Key` header according to the WebSocket spec.
-pub fn hash_key(key: &[u8]) -> String {
+///
-    use sha1::Digest;
+/// Result is a Base64 encoded byte array. `base64(sha1(input))` is always 28 bytes.
-    let mut hasher = sha1::Sha1::new();
+pub fn hash_key(key: &[u8]) -> [u8; 28] {
+    let hash = {
+        use sha1::Digest as _;
 
-    hasher.update(key);
+        let mut hasher = sha1::Sha1::new();
-    hasher.update(WS_GUID.as_bytes());
 
-    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::<u16>::into(CloseCode::Normal));
         assert_eq!(1001u16, Into::<u16>::into(CloseCode::Away));
         assert_eq!(1002u16, Into::<u16>::into(CloseCode::Protocol));

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 {

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),

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: {:?}",
+                    "Invalid challenge response: expected: {:?} received: {:?}",
-                    encoded,
+                    &encoded,
                     key
                 );
+
                 return Err(WsClientError::InvalidChallengeResponse(
                     encoded,
                     hdr_key.clone(),