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

Adding features to ipinfo to support Rustls #60

Merged
merged 4 commits into from
Jan 1, 2025

Conversation

aalkhodiry
Copy link
Contributor

Update dependencies in Cargo.toml to disable default features for reqwest and add new feature flags for OpenSSL and Rustls. This enhances control over the used TLS implementation.

…west and add new feature flags for OpenSSL and Rustls. This enhances control over the used TLS implementation.
- fix tests
- update test assertions for IP details to reflect new expected values for city, region, location, postal code, and timezone.
@aalkhodiry
Copy link
Contributor Author

Changes:

  • Updated reqwest to version 0.12.
  • fix some of the tests that were failing due to IP information changes

@max-ipinfo
Copy link
Contributor

Thanks @aalkhodiry. I will try to review this change this week.

@max-ipinfo
Copy link
Contributor

max-ipinfo commented Dec 30, 2024

@aalkhodiry quick update that I was away for the holiday season. Hoping to taking a look at your PR this week.

Thanks for your patience.

Cargo.toml Outdated
@@ -36,3 +36,8 @@ tokio = { version = "1", features = ["rt-multi-thread", "macros"] }
[profile.release]
overflow-checks = true
lto = true

[features]
default = ["openssl"]
Copy link
Contributor

Choose a reason for hiding this comment

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

I found the following pointers:

@aalkhodiry I believe we should use the following [features] clause instead:

default = ["default-tls"]
default-tls = ["reqwest/default-tls"]
native-tls = ["reqwest/native-tls"]
rustls-tls = ["reqwest/rustls-tls"]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added the suggested features.

[features]
default = ["default-tls"]
default-tls = ["reqwest/default-tls"]
native-tls = ["reqwest/native-tls"]
rustls-tls = ["reqwest/rustls-tls"]

adding these
```
default = ["default-tls"]
default-tls = ["reqwest/default-tls"]
native-tls = ["reqwest/native-tls"]
rustls-tls = ["reqwest/rustls-tls"]
```

as segussted by @max-ipinfo in ipinfo#60 (comment)
@max-ipinfo
Copy link
Contributor

max-ipinfo commented Jan 1, 2025

@aalkhodiry you may need to run a bunch of steps still as CI is detecting a Clippy failure (https://github.com/ipinfo/rust/pull/60/checks?check_run_id=35029056443):

error: the borrowed expression implements the required traits
   --> src/ipinfo.rs:263:19
    |
263 |             .post(&format!("{}/batch", BASE_URL))
    |                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: change this to: `format!("{}/batch", BASE_URL)`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrows_for_generic_args
    = note: `-D clippy::needless-borrows-for-generic-args` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(clippy::needless_borrows_for_generic_args)]`

error: the borrowed expression implements the required traits
   --> src/ipinfo.rs:366:18
    |
366 |             .get(&format!("{}/{}", base_url, ip))
    |                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: change this to: `format!("{}/{}", base_url, ip)`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrows_for_generic_args

error: could not compile `ipinfo` (lib) due to 2 previous errors

Check out https://github.com/ipinfo/rust/blob/master/.github/workflows/rust.yaml

@aalkhodiry
Copy link
Contributor Author

sure I will fix them tomorrow

- Fixes `the borrowed expression implements the required traits`
- Changed string formatting from `format!` to direct string interpolation for POST and GET requests in `ipinfo.rs`.
- This improves readability and maintains consistency in the codebase.
@aalkhodiry
Copy link
Contributor Author

cargo clippy errors are fixed.

@max-ipinfo
Copy link
Contributor

@aalkhodiry I am investigating why some Cargo tests are failing (https://github.com/ipinfo/rust/actions/runs/12571264092):

running 10 tests
test error::tests::iperror_new ... ok
test error::tests::iperrorkind_convert_to_iperror ... ok
test error::tests::iperrorkind_string_values ... ok
test bogon::tests::test_is_bogon ... ok
test ipinfo::tests::ipinfo_config_defaults_reasonable ... ok
test ipinfo::tests::request_headers_are_canonical ... ok
test ipinfo::tests::request_no_token ... ok
test ipinfo::tests::request_multiple_ip ... FAILED
test ipinfo::tests::request_single_ip ... ok
test ipinfo::tests::request_cache_miss_and_hit ... FAILED

failures:

---- ipinfo::tests::request_multiple_ip stdout ----
thread 'ipinfo::tests::request_multiple_ip' panicked at src/ipinfo.rs:538:14:
should lookup: IpError { kind: IpRequestError, description: Some("API token required") }
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- ipinfo::tests::request_cache_miss_and_hit stdout ----
thread 'ipinfo::tests::request_cache_miss_and_hit' panicked at src/ipinfo.rs:603:14:
should lookup: IpError { kind: IpRequestError, description: Some("API token required") }


failures:
    ipinfo::tests::request_cache_miss_and_hit
    ipinfo::tests::request_multiple_ip

@aalkhodiry
Copy link
Contributor Author

It needs an API Token to be available as an environment variable. I found calls the real service in the tests

@max-ipinfo
Copy link
Contributor

max-ipinfo commented Jan 1, 2025

It needs an API Token to be available as an environment variable. I found calls the real service in the tests

@aalkhodiry tests pass locally for me with my token so something is not being passed correctly in the workflow.

I had already updated the token and it's still failing in Github Actions. Investigating further.

@aalkhodiry
Copy link
Contributor Author

I just run it locally and it passed all the test with API Token

@max-ipinfo max-ipinfo mentioned this pull request Jan 1, 2025
@max-ipinfo
Copy link
Contributor

max-ipinfo commented Jan 1, 2025

Found it!

It's due to a fundamental Github restriction, limiting access to Github secrets in workflows triggered by pull_request event (https://docs.github.com/en/actions/writing-workflows/choosing-when-your-workflow-runs/events-that-trigger-workflows#workflows-in-forked-repositories):

With the exception of GITHUB_TOKEN, secrets are not passed to the runner when a workflow is triggered from a forked repository. The GITHUB_TOKEN has read-only permissions in pull requests from forked repositories. For more information, see Automatic token authentication.

More information in https://securitylab.github.com/resources/github-actions-preventing-pwn-requests/

@max-ipinfo max-ipinfo merged commit c1946ac into ipinfo:master Jan 1, 2025
1 check failed
@aalkhodiry
Copy link
Contributor Author

Thank you

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