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

enha: reject unknown clients in the middleware #1939

Merged
merged 1 commit into from
Jan 7, 2025

Conversation

carneiro-cw
Copy link
Contributor

@carneiro-cw carneiro-cw commented Jan 7, 2025

User description

This is the first step towards removing the method wrapper.


PR Type

Enhancement


Description

  • Reject unknown clients in middleware

  • Remove method wrapper for client rejection

  • Implement client identification via headers/URL

  • Update E2E tests for unknown client handling


Changes walkthrough 📝

Relevant files
Enhancement
stratus_error.rs
Add response future conversion for StratusError                   

src/eth/primitives/stratus_error.rs

  • Added imports for futures and jsonrpsee
  • Implemented to_response_future method for StratusError
  • +11/-0   
    rpc_method_wrapper.rs
    Remove client rejection from metrics wrapper                         

    src/eth/rpc/rpc_method_wrapper.rs

    • Removed reject_unknown_client call from metrics wrapper
    +0/-3     
    rpc_middleware.rs
    Implement client rejection in RPC middleware                         

    src/eth/rpc/rpc_middleware.rs

  • Implemented reject_unknown_client function in middleware
  • Updated call method to use new rejection logic
  • +16/-2   
    rpc_server.rs
    Remove client rejection from RPC server methods                   

    src/eth/rpc/rpc_server.rs

  • Removed reject_unknown_client function and its usage
  • Removed client rejection from stratus_get_subscriptions and
    eth_subscribe
  • +1/-18   
    Miscellaneous
    mod.rs
    Remove unused import for client rejection                               

    src/eth/rpc/mod.rs

    • Removed import of reject_unknown_client function
    +0/-1     
    Configuration changes
    hardhat.config.ts
    Update Stratus URL in Hardhat config                                         

    e2e/hardhat.config.ts

    • Removed ?app=e2e from Stratus URL in config
    +1/-1     
    justfile
    Enable debug logging for Stratus in E2E tests                       

    justfile

    • Added RUST_LOG=debug to Stratus startup command
    +1/-1     
    Tests
    e2e-json-rpc.test.ts
    Add E2E tests for unknown client handling                               

    e2e/test/automine/e2e-json-rpc.test.ts

  • Added new tests for unknown client handling
  • Implemented tests for enabling/disabling unknown clients
  • Added checks for various client identification methods
  • +61/-2   

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link

    github-actions bot commented Jan 7, 2025

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Error Handling

    The new reject_unknown_client function returns an Option. Ensure that the error case is properly handled and propagated in all scenarios where this function is called.

    /// Returns an error JSON-RPC response if the client is not allowed to perform the current operation.
    fn reject_unknown_client<'a>(client: &RpcClientApp, id: Id<'_>) -> Option<ResponseFuture<BoxFuture<'a, MethodResponse>>> {
        if client.is_unknown() && !GlobalState::is_unknown_client_enabled() {
            return Some(StratusError::RPC(RpcError::ClientMissing).to_response_future(id));
        }
        None
    }
    Potential Performance Impact

    The removal of the reject_unknown_client check from individual RPC methods might impact performance if the check is now being done for every request in the middleware. Consider the trade-offs between security and performance.

    fn stratus_state(_: Params<'_>, ctx: &RpcContext, _: &Extensions) -> Result<JsonValue, StratusError> {
        Ok(GlobalState::get_global_state_as_json(ctx))
    }
    
    async fn stratus_get_subscriptions(_: Params<'_>, ctx: Arc<RpcContext>, _: Extensions) -> Result<JsonValue, StratusError> {
        // NOTE: this is a workaround for holding only one lock at a time
        let pending_txs = serde_json::to_value(ctx.subs.new_heads.read().await.values().collect_vec()).expect_infallible();
        let new_heads = serde_json::to_value(ctx.subs.pending_txs.read().await.values().collect_vec()).expect_infallible();
        let logs = serde_json::to_value(ctx.subs.logs.read().await.values().flat_map(HashMap::values).collect_vec()).expect_infallible();

    Copy link

    github-actions bot commented Jan 7, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    General
    Add logging and metrics for rejected unknown client requests to improve monitoring and debugging capabilities

    Consider adding logging or metrics collection in the reject_unknown_client function
    to track rejected requests from unknown clients. This would help in monitoring and
    debugging.

    src/eth/rpc/rpc_middleware.rs [160-165]

     fn reject_unknown_client<'a>(client: &RpcClientApp, id: Id<'_>) -> Option<ResponseFuture<BoxFuture<'a, MethodResponse>>> {
         if client.is_unknown() && !GlobalState::is_unknown_client_enabled() {
    +        tracing::warn!("Rejected request from unknown client: {:?}", id);
    +        metrics::increment_counter!("unknown_client_rejections");
             return Some(StratusError::RPC(RpcError::ClientMissing).to_response_future(id));
         }
         None
     }
    Suggestion importance[1-10]: 7

    Why: Adding logging and metrics for rejected unknown client requests would significantly improve monitoring and debugging capabilities, which is valuable for maintaining and troubleshooting the system.

    7
    Use a more specific error type for unknown client rejection to improve error handling and clarity

    Consider using a more specific error type instead of
    StratusError::RPC(RpcError::ClientMissing) in the reject_unknown_client function.
    This would provide more clarity about the nature of the error.

    src/eth/rpc/rpc_middleware.rs [160-165]

     fn reject_unknown_client<'a>(client: &RpcClientApp, id: Id<'_>) -> Option<ResponseFuture<BoxFuture<'a, MethodResponse>>> {
         if client.is_unknown() && !GlobalState::is_unknown_client_enabled() {
    -        return Some(StratusError::RPC(RpcError::ClientMissing).to_response_future(id));
    +        return Some(StratusError::UnknownClientRejected.to_response_future(id));
         }
         None
     }
    Suggestion importance[1-10]: 5

    Why: The suggestion proposes using a more specific error type, which could improve error handling and clarity. However, it requires creating a new error type, which may not be necessary if the current error is sufficiently clear.

    5

    @carneiro-cw carneiro-cw merged commit a22f796 into main Jan 7, 2025
    37 checks passed
    @carneiro-cw carneiro-cw deleted the remove_method_wrapper branch January 7, 2025 14:50
    @gabriel-aranha-cw
    Copy link
    Contributor

    Final benchmark:
    Run ID: bench-3580726886

    Git Info:

    Configuration:

    • Target Account Strategy: Default

    RPS Stats: Max: 1251.00, Min: 919.00, Avg: 1114.58, StdDev: 51.00
    TPS Stats: Max: 1245.00, Min: 892.00, Avg: 1080.42, StdDev: 60.58

    Plot: View Plot

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

    Successfully merging this pull request may close these issues.

    2 participants