Skip to content

Support -insecure-skip-verify flag for LSIF upload#559

Merged
Strum355 merged 3 commits into
mainfrom
nsc/lsif-upload-insecureskipverify
Jun 25, 2021
Merged

Support -insecure-skip-verify flag for LSIF upload#559
Strum355 merged 3 commits into
mainfrom
nsc/lsif-upload-insecureskipverify

Conversation

@Strum355

@Strum355 Strum355 commented Jun 25, 2021

Copy link
Copy Markdown
Contributor

As we previously used a different API client as was provided in the src-cli codebase, we did not take into account the -skip-insecure-verify flag. This affected a customer as they used self-signed certs.

This PR passes the src-cli configured API client that honours the flag to the LSIF upload function that resides in sg/sg/lib. The only part of the API client that is consumed is the direct pass-through method, Do(...)

Related to https://github.com/sourcegraph/sourcegraph/pull/22399 and will require bumping go.mod here when it is merged.

cc @efritz for 👀 after PTO

@Strum355

Strum355 commented Jun 25, 2021

Copy link
Copy Markdown
Contributor Author

Note tests are failing due to go.mod pointing at the old sg/sg/lib API while the related PR is unmerged, will be rerun once it is merged

Comment thread cmd/src/lsif_upload.go
client := api.NewClient(api.ClientOpts{
Flags: lsifUploadFlags.apiFlags,
})

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It might be a good idea to log a warning if the client is initialized without TLS validation.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thats a good idea, I'll follow up with a separate PR for that, as those changes would be more wide-reaching across teams 🙂

@Strum355 Strum355 merged commit 7051ea1 into main Jun 25, 2021
@Strum355 Strum355 deleted the nsc/lsif-upload-insecureskipverify branch June 25, 2021 20:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants