Skip to content

fix(op-signer): allow anon auth when mTLS is disabled (for local testing only) #263

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

ddaws
Copy link
Contributor

@ddaws ddaws commented Mar 27, 2025

Description

This MR adds func NewAnonMiddleware() oprpc.Middleware which sets the ClientInfo.ClientName to anonymous when mTLS auth is disabled. This is needed to allow disabling TLS auth locally.

Tests

You can test by creating a config.yaml like

provider: LOCAL
auth:
  - name: anonymous          # The key needs to be explicitly allowed for anonymous usage
    key: tls/ec_private.pem

Then post to op-signer to see the signing results

$ curl -X POST -H "Content-Type: application/json" -d @test-rpc.json http://localhost:8080
[
  {
    "jsonrpc": "2.0",
    "id": "1",
    "result": "0x02f89d82053980010182753094000000000000000000000000000000000000aaaa0180f838f794000000000000000000000000000000000000aaaae1a0000000000000000000000000000000000000000000000000000000000000000001a072b0f296886fe5eeb4b18cba71236063dfe520789d8fd77fa3a8ede175ba5c68a006a19fe680bda685effac26c18e661d7fd0f578fafb14c24b1e72ff80dc7cd45"
  },
  {
    "jsonrpc": "2.0",
    "id": "1",
    "error": {
      "code": -32010,
      "message": "nonce not specified"
    }
  }
]

@codecov-commenter
Copy link

codecov-commenter commented Mar 27, 2025

Codecov Report

Attention: Patch coverage is 37.50000% with 20 lines in your changes missing coverage. Please review.

Project coverage is 53.68%. Comparing base (d6d181a) to head (82497e7).

Files with missing lines Patch % Lines
op-signer/service/auth.go 0.00% 9 Missing ⚠️
op-signer/app.go 0.00% 8 Missing ⚠️
op-signer/provider/provider.go 76.92% 2 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #263      +/-   ##
==========================================
- Coverage   53.76%   53.68%   -0.09%     
==========================================
  Files          67       67              
  Lines        7923     7947      +24     
==========================================
+ Hits         4260     4266       +6     
- Misses       3381     3398      +17     
- Partials      282      283       +1     
Flag Coverage Δ
op-signer 46.83% <37.50%> (-0.54%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
op-signer/provider/config.go 69.23% <100.00%> (+0.80%) ⬆️
op-signer/provider/provider.go 45.83% <76.92%> (+8.33%) ⬆️
op-signer/app.go 0.00% <0.00%> (ø)
op-signer/service/auth.go 58.06% <0.00%> (-23.76%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@ddaws ddaws marked this pull request as ready for review March 29, 2025 06:00
@ddaws ddaws requested a review from a team as a code owner March 29, 2025 06:00
@ddaws ddaws requested a review from edobry March 29, 2025 06:00
@edobry
Copy link
Contributor

edobry commented Apr 2, 2025

LGTM overall, tested & confirmed working.

however, what should happen if the config file's auths list has multiple anonymous entries? i tested this and it seemed to just pick one of the entries arbitrarily, should there be some validation here?

additionally, seems like its possible to add auths entries with a non-anonymous name when TLS is disabled, which will never get used; similarly, should there be validation around this?

@ddaws
Copy link
Contributor Author

ddaws commented Apr 9, 2025

however, what should happen if the config file's auths list has multiple anonymous entries? i tested this and it seemed to just pick one of the entries arbitrarily, should there be some validation here?

I think this is fine and there doesn't need to be validation added here. This would be the same if there were duplicate hostname entries, so I would expect similar behavior.

additionally, seems like its possible to add auths entries with a non-anonymous name when TLS is disabled, which will never get used; similarly, should there be validation around this?

I don't think we need to validate for this. I think this is fine, and they just wont be usable, and probably shouldn't be usable.

I'm happy to add some validation in another PR. I think this should just result in warnings being logged

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.

3 participants