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

Allow to ignore validation of TLS certificates #125

Merged
merged 5 commits into from
Feb 16, 2024

Conversation

seqre
Copy link
Contributor

@seqre seqre commented Feb 9, 2024

This PR closes #116. If there's a need for tests or anything else, I can add them.

There are two approaches I see to CLI arguments:

  1. (implemented right now) For each command that interacts with certificates, add an argument to its defined arguments and pass it to the functions. It's more local, closer to actual use, yet adds more places where it needs to be maintained.
  2. Consider this to be a global argument. The plus of this approach is that there is only one place for the user to set it, and it could be easily used if additional clients were to be added in the future (e.g. GitLab or something else). The disadvantage is that it would require more modification to the code as GlobalArgs is not always passed that deep. Also, it wouldn't apply to all subcommands, so it wouldn't be truly "global".

@seqre seqre marked this pull request as draft February 9, 2024 18:49
@seqre seqre marked this pull request as ready for review February 10, 2024 23:56
Copy link
Collaborator

@bradlarsen bradlarsen left a comment

Choose a reason for hiding this comment

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

Thanks @seqre! This is looking really good.

I suspect that these changes will fix enumeration of GitHub API servers that may be behind self-signed or otherwise invalid certificates. But I don't believe the behavior of ignoring certificates is propagated into the code that actually clones repositories, which I think is the actual problem in #116.

Some background

Nosey Parker shells out to git to do its cloning; you can see this at crates/noseyparker/src/git_binary.rs. I had earlier tried to use both libgit2 and the Rust-native gix implementations instead, but both of those ran into problems when trying to do bare or mirror clones.

Nosey Parker's git wrapper goes out of its way to ignore any user- or system-specific git configuration. It also uses some convoluted shell scripting to propagate any provided GitHub access token to the underlying git binary.

Nice-to-have

It would be nice if there were a couple new integration test cases:

  1. One that tests that the github repos list command works against a server with an invalid certificate
  2. One that tests that the scan --git-url https://some-invalid-cert-server.example.com/foo.git behaves as expected with and without the --ignore-certs CLI option

I realize that these test cases may be difficult to express, as they depend on complicated external systems, so I won't require them.

To finish this PR

  1. When the new --ignore-certs CLI option is specified, the git wrapper probably needs to be updated to set the GIT_SSL_NO_VERIFY=1 environment variable, probably here.

  2. Run a manual test that scan --git-url https://some-invalid-cert-server.example.com/foo.git works as intended both with and without the --ignore-certs CLI option

  3. If not adding new integration tests mentioned in the "Nice-to-have" section, add a TODO comment about those scenarios here

CHANGELOG.md Outdated Show resolved Hide resolved
@seqre
Copy link
Contributor Author

seqre commented Feb 16, 2024

I've applied review comments and tested manually that the flag works against self-hosted Gitea with self-signed SSL certificates. Output without the --ignore-certs:

2024-02-16T03:20:50.135120Z ERROR noseyparker_cli::cmd_scan: Failed to clone https://localhost:3000/admin2/test to /home/seqre/Projects/noseyparker/np.test/clones/https/localhost:3000/admin2/test: git execution failed
code=exit status: 128
stdout=```
```
stderr=```
Cloning into bare repository '/home/seqre/Projects/noseyparker/np.test/clones/https/localhost:3000/admin2/test'...
fatal: unable to access 'https://localhost:3000/admin2/test/': SSL certificate problem: self-signed certificate
```

Once the --ignore-certs flag was supplied, Noseyparker executed without issues.

Regarding the nice-to-have tests, I'd gladly create them, but I have no idea how to approach them. I don't think we want to spin up the git server in the CI pipeline or connect to some random git server on the Internet that doesn't have proper certificates.

@bradlarsen
Copy link
Collaborator

Regarding the nice-to-have tests, I'd gladly create them, but I have no idea how to approach them. I don't think we want to spin up the git server in the CI pipeline or connect to some random git server on the Internet that doesn't have proper certificates.

Yeah, I don't know an easy way to write those tests either. Don't worry about adding those now.

@bradlarsen bradlarsen merged commit 1536e05 into praetorian-inc:main Feb 16, 2024
8 checks passed
@bradlarsen
Copy link
Collaborator

Thanks @seqre!

@seqre seqre deleted the ignore-certs branch February 16, 2024 19:29
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.

Add an option to ignore invalid certificates when cloning Git repos
2 participants