Enforce API preconditions

- Handshake must proceed other requests
- Other requests fail when post-handshake
  traffic rules result in immediately
  exhausted client

As part of this commit, client_session_id
is no longer an optional base API param.
Making this optional no longer seems
useful since, even before this change, a
handshake fails if it's not supplied.

psiphon/server/api.go

@@ -108,6 +108,46 @@ func dispatchAPIRequestHandler(
 		}
 	}()
 
+	// Before invoking the handlers, enforce some preconditions:
+	//
+	// - A handshake request must preceed any other requests.
+	// - When the handshake results in a traffic rules state where
+	//   the client is immediately exhausted, no requests
+	//   may succeed. This case ensures that blocked clients do
+	//   not log "connected", etc.
+	//
+	// Note that sshClient.setHandshakeState enforces that only a
+	// single handshake is made; enforcing that there ensures no
+	// race condition even if concurrent requests are in flight.
+
+	if name != protocol.PSIPHON_API_HANDSHAKE_REQUEST_NAME {
+
+		// TODO: same session-ID-lookup TODO in handshakeAPIRequestHandler
+		// applies here.
+
+		sessionID, err := getStringRequestParam(params, "client_session_id")
+		if err == nil {
+			// Note: follows/duplicates baseRequestParams validation
+			if !isHexDigits(support, sessionID) {
+				err = errors.New("invalid param: client_session_id")
+			}
+		}
+		if err != nil {
+			return nil, common.ContextError(err)
+		}
+
+		completed, exhausted, err := support.TunnelServer.GetClientHandshaked(sessionID)
+		if err != nil {
+			return nil, common.ContextError(err)
+		}
+		if !completed {
+			return nil, common.ContextError(errors.New("handshake not completed"))
+		}
+		if exhausted {
+			return nil, common.ContextError(errors.New("exhausted after handshake"))
+		}
+	}
+
 	switch name {
 	case protocol.PSIPHON_API_HANDSHAKE_REQUEST_NAME:
 		return handshakeAPIRequestHandler(support, apiProtocol, geoIPData, params)
@@ -147,8 +187,6 @@ func handshakeAPIRequestHandler(
 			params,
 			baseRequestParams))
 
-	// Note: ignoring param format errors as params have been validated
-
 	sessionID, _ := getStringRequestParam(params, "client_session_id")
 	sponsorID, _ := getStringRequestParam(params, "sponsor_id")
 	clientVersion, _ := getStringRequestParam(params, "client_version")
@@ -509,7 +547,7 @@ const (
 // is specified, in which case an array of string is expected.
 var baseRequestParams = []requestParamSpec{
 	requestParamSpec{"server_secret", isServerSecret, requestParamNotLogged},
-	requestParamSpec{"client_session_id", isHexDigits, requestParamOptional | requestParamNotLogged},
+	requestParamSpec{"client_session_id", isHexDigits, requestParamNotLogged},
 	requestParamSpec{"propagation_channel_id", isHexDigits, 0},
 	requestParamSpec{"sponsor_id", isHexDigits, 0},
 	requestParamSpec{"client_version", isIntString, 0},

psiphon/server/tunnelServer.go

@@ -225,6 +225,14 @@ func (server *TunnelServer) SetClientHandshakeState(
 	return server.sshServer.setClientHandshakeState(sessionID, state)
 }
 
+// GetClientHandshaked indicates whether the client has completed a handshake
+// and whether its traffic rules are immediately exhausted.
+func (server *TunnelServer) GetClientHandshaked(
+	sessionID string) (bool, bool, error) {
+
+	return server.sshServer.getClientHandshaked(sessionID)
+}
+
 // SetEstablishTunnels sets whether new tunnels may be established or not.
 // When not establishing, incoming connections are immediately closed.
 func (server *TunnelServer) SetEstablishTunnels(establish bool) {
@@ -660,6 +668,22 @@ func (sshServer *sshServer) setClientHandshakeState(
 	return nil
 }
 
+func (sshServer *sshServer) getClientHandshaked(
+	sessionID string) (bool, bool, error) {
+
+	sshServer.clientsMutex.Lock()
+	client := sshServer.clients[sessionID]
+	sshServer.clientsMutex.Unlock()
+
+	if client == nil {
+		return false, false, common.ContextError(errors.New("unknown session ID"))
+	}
+
+	completed, exhausted := client.getHandshaked()
+
+	return completed, exhausted, nil
+}
+
 func (sshServer *sshServer) stopClients() {
 
 	sshServer.clientsMutex.Lock()
@@ -1497,6 +1521,40 @@ func (sshClient *sshClient) setHandshakeState(state handshakeState) error {
 	return nil
 }
 
+// getHandshaked returns whether the client has completed a handshake API
+// request and whether the traffic rules that were selected after the
+// handshake immediately exhaust the client.
+//
+// When the client is immediately exhausted it will be closed; but this
+// takes effect asynchronously. The "exhausted" return value is used to
+// prevent API requests by clients that will close.
+func (sshClient *sshClient) getHandshaked() (bool, bool) {
+	sshClient.Lock()
+	defer sshClient.Unlock()
+
+	completed := sshClient.handshakeState.completed
+
+	exhausted := false
+
+	// Notes:
+	// - "Immediately exhausted" is when CloseAfterExhausted is set and
+	//   either ReadUnthrottledBytes or WriteUnthrottledBytes starts from
+	//   0, so no bytes would be read or written. This check does not
+	//   examine whether 0 bytes _remain_ in the ThrottledConn.
+	// - This check is made against the current traffic rules, which
+	//   could have changed in a hot reload since the handshake.
+
+	if completed &&
+		*sshClient.trafficRules.RateLimits.CloseAfterExhausted == true &&
+		(*sshClient.trafficRules.RateLimits.ReadUnthrottledBytes == 0 ||
+			*sshClient.trafficRules.RateLimits.WriteUnthrottledBytes == 0) {
+
+		exhausted = true
+	}
+
+	return completed, exhausted
+}
+
 // setTrafficRules resets the client's traffic rules based on the latest server config
 // and client properties. As sshClient.trafficRules may be reset by a concurrent
 // goroutine, trafficRules must only be accessed within the sshClient mutex.