[GitHub] Add a GraphQL client to the connector

[rgmz]

Description:

This introduces a latent GraphQL client without making any functional changes. The GraphQL API is required for certain features (e.g., #1906), and it is already being manually called in at least once place:

trufflehog/pkg/sources/github_experimental/object_discovery.go

Lines 429 to 432 in 6d1c59f

	req, err := http.NewRequest("POST", "https://api.github.com/graphql", bytes.NewBuffer(requestBody))
	if err != nil {
	return nil, fmt.Errorf("failed to create request: %w", err)
	}

shurcooL/githubv4 is the library recommended by the authors of google/go-github.

Checklist:

• Tests passing (make test-community)?
• Lint passing (make lint this requires golangci-lint)?

[rosecodym]

Pretty cool! It looks like these clients are all unauthenticated - out of curiosity, do you foresee them ever being authenticated?

[rosecodym]

Shouldn't this logger name be added way higher than the call stack, at entry to the overall github code? I presume you didn't do that because github.com/trufflesecurity/trufflehog/v3/pkg/context doesn't support WithName, but I don't think kludging around that by adding contextual information in the "wrong" spot like this is a good solution overall because we've seen it contribute to bifurcation of our loggers (where a chunk of code is using two loggers at once, each with different contextual information), which in turn has caused concrete debugging problems.

To be clear, I'm not worried that this specific code change will cause an issue - I'm worried that in nine months somebody else is going to copy the pattern somewhere else without thinking too hard about it and create a maintainability problem. If you want to add this piece of context, I think it's safer in the long term to add it at the entry to the github code either using WithValue or with a new WithName that you add to our logging package.

[rgmz]

Yeah, the current implementation doesn't make it easy to have a named logger for a package. You either have to call .WithName() repeatedly or pass a logger separately -- neither are ideal.

I think it's safer in the long term to add ... a new WithName that you add to our logging package.

I can try to tackle that in a different PR.

[rgmz]

WithName functionality can't be added to the logger Context. Calls to WithName are appended to the logger, meaning that a context passed between packages could become trufflehog.github.git.detector.etc.

The best approach is probably a simple helper function in the package.

[mcastorina]

I'm not sure if this is what you're going for, but a logger in a context can be modified via:

ctx = context.WithLogger(ctx, ctx.Logger().WithName("github"))

Not the most ergonomic, but imo not completely foreign (might be just me since I wrote trufflehog/pkg/context and trufflehog/pkg/log though).

[rgmz]

I'm not sure if this is what you're going for, but a logger in a context can be modified via:

ctx = context.WithLogger(ctx, ctx.Logger().WithName("github"))

Not the most ergonomic, but imo not completely foreign (might be just me since I wrote trufflehog/pkg/context and trufflehog/pkg/log though).

I think this would still have the issue of 'unbounded' logger names:

Calls to WithName are appended to the logger, meaning that a context passed between packages could become trufflehog.github.git.detector.etc.

There's no way to unset or overwrite the logger name that I'm aware of.

[rgmz]

Pretty cool! It looks like these clients are all unauthenticated - out of curiosity, do you foresee them ever being authenticated?

It should inherit authentication from the HTTP client's transport:

trufflehog/pkg/sources/github/connector_token.go

Lines 34 to 39 in c067504

	httpClient := common.RetryableHTTPClientTimeout(int64(httpTimeoutSeconds))
	tokenSource := oauth2.StaticTokenSource(&oauth2.Token{AccessToken: token})
	httpClient.Transport = &oauth2.Transport{
	Base: httpClient.Transport,
	Source: tokenSource,
	}

[rosecodym]

🤦 no idea how i missed that