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

introduce a flag to turn off SSRF protection for local development #16622

Draft
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

jinhoonbang
Copy link
Contributor

  • Introduce a flag to turn off SSRF protection. This helps with local development when the gateway needs to make outgoing calls to localhost servers or docker containers.
  • SSRF protection is enabled by default.

@cedric-cordenier
Copy link
Contributor

@jinhoonbang Can you use the AllowedIPs setting instead for this? That basically achieves whan you want

@jinhoonbang
Copy link
Contributor Author

jinhoonbang commented Feb 27, 2025

@cedric-cordenier Is AllowedIPs helpful for local testing? We face some of these challenges maybe because we have webapi target in our workflows:

  1. host IP can be dynamic
  2. safeURL doesn't seem to support allowlisting CIDRs. Local tests that use docker container would need to allowlist CIDR block like 172.17.0.0/16`

I see some workarounds like this. But found this new flag to be simpler

Copy link
Contributor

github-actions bot commented Feb 27, 2025

AER Report: CI Core

aer_workflow , commit , Detect Changes , Scheduled Run Frequency , Clean Go Tidy & Generate , Core Tests (go_core_tests) , GolangCI Lint (.) , Core Tests (go_core_tests_integration) , Core Tests (go_core_ccip_deployment_tests) , Core Tests (go_core_fuzz) , test-scripts , Core Tests (go_core_race_tests) , lint , SonarQube Scan

1. GolangCI Lint job failed: Check Golangci-lint Matrix Results

Source of Error:
Check Golangci-lint Matrix Results	2025-02-28T06:39:04.9066331Z ##[error]Process completed with exit code 1.
**Why**: The GolangCI Lint job failed due to one or more linting issues detected in the codebase.

Suggested fix: Review the detailed linting report and fix the identified issues in the codebase to pass the linting checks.

2. Variable shadowing issue: Golang Lint (.)

Source of Error:
Golang Lint (.)	2025-02-28T06:38:46.9738361Z ##[error]core/services/gateway/network/httpclient.go:94:6: shadow: declaration of "client" shadows declaration at line 85 (govet)
Golang Lint (.)	2025-02-28T06:38:46.9739569Z 	var client client
Golang Lint (.)	2025-02-28T06:38:46.9740157Z 	 ^
**Why**: The error indicates that a variable named `client` declared at line 94 shadows another variable with the same name declared at line 85.

Suggested fix: Rename one of the client variables to avoid shadowing and ensure clarity in the code.

3. Test failure in logprovider: Run tests

Source of Error:
Run tests	2025-02-28T06:57:56.7191525Z FAIL	github.com/smartcontractkit/chainlink/v2/core/services/ocr2/plugins/ocr2keeper/evmregistry/v21/logprovider	1200.392s
Run tests	2025-02-28T06:57:57.7086270Z FAIL
Run tests	2025-02-28T06:57:57.7595683Z Encountered test failures.
Run tests	2025-02-28T06:57:57.7596071Z go_core_tests exiting with code 1
Run tests	2025-02-28T06:57:57.7605869Z ##[error]Process completed with exit code 1.
**Why**: The test suite for `logprovider` in the `ocr2keeper` plugin failed, causing the overall test run to exit with a non-zero status.

Suggested fix: Investigate the specific test failures in the logprovider package and address the issues causing the tests to fail. This may involve debugging the test cases or the implementation code.

AER Report: Operator UI CI ran successfully ✅

aer_workflow , commit

var _ job.Delegate = (*Delegate)(nil)

func NewDelegate(legacyChains legacyevm.LegacyChainContainer, ks keystore.Eth, ds sqlutil.DataSource, lggr logger.Logger) *Delegate {
func NewDelegate(config Config, legacyChains legacyevm.LegacyChainContainer, ks keystore.Eth, ds sqlutil.DataSource, lggr logger.Logger) *Delegate {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will other fields from the config need to be accessed eventually? If not, does it make more sense to add a single boolean param to the constructor here instead? And maybe further along the param list?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potentially other fields from the config may be needed in the future. Mostly just borrowed from the pattern in other delegates (code). Created a narrow interface here so that it doesn't access fields from anything other than Insecure config.

@cl-sonarqube-production
Copy link

Quality Gate failed Quality Gate failed

Failed conditions
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube

Catch issues before they fail your Quality Gate with our IDE extension SonarQube IDE SonarQube IDE

@Tofel
Copy link
Contributor

Tofel commented Feb 28, 2025

@jinhoonbang curious: can't the same be achieved by passing a list of allowed IPs to the gateway job? This is what I am doing in my e2e tests to allow nodes to access resources from the host machine. Here's the code:
https://github.com/smartcontractkit/chainlink/blob/develop/core/services/gateway/network/httpclient.go#L30
https://github.com/smartcontractkit/chainlink/blob/develop/system-tests/lib/cre/don/jobs/definitions.go#L139

@jinhoonbang
Copy link
Contributor Author

jinhoonbang commented Feb 28, 2025

yes. we had to add this to gateway job specs to get things working. : AllowedIps = ["172.18.0.13","172.18.0.14","172.18.0.15","172.18.0.16","172.18.0.17","172.18.0.18", "192.168.65.254", "::1", "127.0.0.1", "%s"] It requires a lot of trial and error and still think this PR would make our local testing easier unless SafeURL supports allowlisting by CIDR block

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.

4 participants