Skip to content

v1.3.0 #27

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

Merged
merged 15 commits into from
Jun 12, 2025
Merged

v1.3.0 #27

merged 15 commits into from
Jun 12, 2025

Conversation

spbsoluble
Copy link
Collaborator

@spbsoluble spbsoluble commented Jun 4, 2025

CHANGELOG

Features

  • Add support for fetching an oauth2 token using the client_credentials grant type without connecting to Keyfactor Command.
  • Add placeholders for omitted Authorization header in the curl command string output in trace logging.

Bug Fixes

  • Log curl command string at trace level after request is sent to include any transport mutations.

Chores

  • Bump Go version to 1.24.

@spbsoluble spbsoluble changed the title Curl output fix v1.2.1 Jun 6, 2025
@spbsoluble spbsoluble requested a review from Copilot June 6, 2025 18:33
Copilot

This comment was marked as outdated.

@spbsoluble spbsoluble requested review from irby and Copilot June 6, 2025 18:34
Copilot

This comment was marked as outdated.

@spbsoluble spbsoluble changed the title v1.2.1 v1.3.0 Jun 12, 2025
@spbsoluble spbsoluble changed the base branch from release-v1.2 to release-v1.3 June 12, 2025 17:11
@spbsoluble spbsoluble requested a review from Copilot June 12, 2025 17:11
Copilot

This comment was marked as outdated.

return nil, fmt.Errorf("client ID, client secret, and token URL must be provided")
}

config := &clientcredentials.Config{
Copy link

Choose a reason for hiding this comment

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

This is a good callout. We are setting the skip verify in the test suite. Should we use this value?

Copilot

This comment was marked as outdated.

Copilot

This comment was marked as outdated.

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR prepares v1.3.0 by upgrading the Go toolchain, adding client-credentials OAuth2 support, enhancing curl-style trace logging with masked Authorization headers, and bumping module dependencies.

  • Bump Go version to 1.24 and update indirect dependencies
  • Implement GetAccessToken on CommandConfigOauth and improve OAuth2 transport logging
  • Enhance RequestToCurl to mask credentials and add new tests

Reviewed Changes

Copilot reviewed 13 out of 14 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
go.mod Bump to Go 1.24 and update SDK/dependency versions
auth_providers/auth_oauth.go Add GetAccessToken, debug logging in transport RoundTrip
auth_providers/auth_oauth_test.go Add TestCommandConfigOauth_GetAccessToken & untrusted cert tests
auth_providers/auth_core.go Move curl logging after request, mask Authorization headers
auth_providers/auth_core_test.go Add TestRequestToCurl for Basic/Bearer auth header masking
auth_providers/auth_basic_test.go Update skip-test env var for basic auth tests
.github/workflows/go_tests.yml Update Go version and new test env vars
.github/config/** Expose new Terraform variables for ses_2441 environment
Files not reviewed (1)
  • .github/config/.terraform.lock.hcl: Language not supported
Comments suppressed due to low confidence (5)

auth_providers/auth_core.go:521

  • Using filepath.Dir(outputPath) creates the parent directory rather than the intended outputPath directory. If outputPath is meant as a directory, create it directly with MkdirAll(outputPath, ...).
if dirErr := os.MkdirAll(filepath.Dir(outputPath), os.ModePerm); dirErr != nil {

auth_providers/auth_core.go:526

  • Removing the hostname-based .crt filename means outputFile is now just the path. If callers expect <hostname>.crt, reintroduce suffix logic or clearly document that full file paths must be provided.
outputFile := filepath.Join(outputPath)

auth_providers/auth_oauth_test.go:266

  • [nitpick] Using very generic substrings like "fail" and "invalid" may match unexpected errors; consider using more specific markers (e.g., "unauthorized_client") to validate the exact failure reason.
"oauth2", "fail", "invalid", "client",

auth_providers/auth_oauth_test.go:367

  • [nitpick] This test uses authOauthTest which also invokes Authenticate. If the goal is only to validate GetAccessToken, extract a dedicated test for GetAccessToken to avoid unintended network calls to Authenticate.
authOauthTest(t, "w/ GetAccessToken w/ full params variables", false, fullParamsConfig)

auth_providers/auth_basic_test.go:29

  • The skip condition for basic-auth tests now checks TEST_KEYFACTOR_OAUTH instead of the intended KEYFACTOR_KC_AUTH; this will incorrectly skip basic auth tests when OAuth is enabled.
if os.Getenv("TEST_KEYFACTOR_OAUTH") == "1" || os.Getenv("TEST_KEYFACTOR_OAUTH") == "true" {

@spbsoluble spbsoluble merged commit 27c5619 into release-v1.3 Jun 12, 2025
31 of 33 checks passed
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