diff --git a/src/clis/nvcf-cli/internal/openbao/client.go b/src/clis/nvcf-cli/internal/openbao/client.go index 7cbe658b3a..1f3d246180 100644 --- a/src/clis/nvcf-cli/internal/openbao/client.go +++ b/src/clis/nvcf-cli/internal/openbao/client.go @@ -25,6 +25,7 @@ import ( "fmt" "net/http" "os/exec" + "strconv" "strings" "time" @@ -60,6 +61,12 @@ type Config struct { // instead of a hard failure. var ErrPKICertificateNotFound = errors.New("openbao: pki certificate not found") +const ( + curlHTTPStatusMarker = "__NVCF_HTTP_STATUS__:" + curlHTTPContentTypeMarker = "__NVCF_HTTP_CONTENT_TYPE__:" + maxOpenBaoHTTPErrorBody = 512 +) + // JWTAuthRequest represents the JWT authentication request to OpenBao type JWTAuthRequest struct { Role string `json:"role"` @@ -87,6 +94,14 @@ type pkiCertificateResponse struct { Errors []string `json:"errors"` } +// pkiCertificateHTTPResponse captures the metadata needed to classify and +// report an OpenBao PKI certificate response. +type pkiCertificateHTTPResponse struct { + StatusCode int + ContentType string + Body string +} + // NewClient creates a new OpenBao client func NewClient(config *Config, k8sClient *k8s.Client) *Client { return &Client{ @@ -397,9 +412,15 @@ func (c *Client) generateUserJWTTokenWithSubject(ctx context.Context, vaultToken // text suitable for a public trust bundle. func (c *Client) ReadPKICertificatePEM(ctx context.Context, pkiPath string) (string, error) { readURL := strings.TrimRight(c.config.OpenBaoURL, "/") + "/v1/" + strings.Trim(pkiPath, "/") + "/cert/ca" - curlArgs := []string{"curl", "-sS", readURL} - return readPKICertificatePEM(ctx, 3, 2*time.Second, func(ctx context.Context) (string, error) { - return c.executeKubectlRun(ctx, "openbao-pki-root-ca", curlArgs) + writeOut := "\n" + curlHTTPStatusMarker + "%{http_code}\n" + + curlHTTPContentTypeMarker + "%{content_type}\n" + curlArgs := []string{"curl", "-sS", "--write-out", writeOut, readURL} + return readPKICertificatePEM(ctx, 3, 2*time.Second, func(ctx context.Context) (pkiCertificateHTTPResponse, error) { + output, err := c.executeKubectlRun(ctx, "openbao-pki-root-ca", curlArgs) + if err != nil { + return pkiCertificateHTTPResponse{}, err + } + return pkiCertificateHTTPResponseFromOutput(output) }) } @@ -407,22 +428,36 @@ func readPKICertificatePEM( ctx context.Context, attempts int, retryDelay time.Duration, - read func(context.Context) (string, error), + read func(context.Context) (pkiCertificateHTTPResponse, error), ) (string, error) { if ctx == nil { ctx = context.Background() } for attempt := 1; attempt <= attempts; attempt++ { - output, err := read(ctx) + response, err := read(ctx) if err != nil { return "", fmt.Errorf("reading OpenBao PKI certificate: %w", err) } - pem, err := rootCAPEMFromOpenBaoResponse(output) + pem, err := rootCAPEMFromOpenBaoResponse(response.Body) + if response.StatusCode != http.StatusOK { + if errors.Is(err, ErrPKICertificateNotFound) && + !retryablePKICertificateHTTPStatus(response.StatusCode) { + return "", err + } + httpErr := pkiCertificateHTTPError(response) + if !retryablePKICertificateHTTPStatus(response.StatusCode) || attempt == attempts { + return "", httpErr + } + if err := waitForPKICertificateRetry(ctx, retryDelay); err != nil { + return "", err + } + continue + } if err == nil { return pem, nil } var syntaxErr *json.SyntaxError - retryable := strings.TrimSpace(output) == "" || errors.As(err, &syntaxErr) + retryable := strings.TrimSpace(response.Body) == "" || errors.As(err, &syntaxErr) if !retryable || attempt == attempts { return "", err } @@ -433,6 +468,63 @@ func readPKICertificatePEM( return "", fmt.Errorf("read OpenBao PKI certificate without an attempt") } +func pkiCertificateHTTPResponseFromOutput(output string) (pkiCertificateHTTPResponse, error) { + var response pkiCertificateHTTPResponse + var bodyLines []string + statusFound := false + + for _, line := range strings.Split(output, "\n") { + switch { + case strings.HasPrefix(line, curlHTTPStatusMarker): + statusText := strings.TrimSpace(strings.TrimPrefix(line, curlHTTPStatusMarker)) + statusCode, err := strconv.Atoi(statusText) + if err != nil { + return pkiCertificateHTTPResponse{}, fmt.Errorf("parse OpenBao PKI HTTP status %q: %w", statusText, err) + } + response.StatusCode = statusCode + statusFound = true + case strings.HasPrefix(line, curlHTTPContentTypeMarker): + response.ContentType = strings.TrimSpace(strings.TrimPrefix(line, curlHTTPContentTypeMarker)) + default: + bodyLines = append(bodyLines, line) + } + } + if !statusFound { + return pkiCertificateHTTPResponse{}, fmt.Errorf("OpenBao PKI response missing HTTP status") + } + response.Body = strings.TrimSpace(strings.Join(bodyLines, "\n")) + return response, nil +} + +func pkiCertificateHTTPError(response pkiCertificateHTTPResponse) error { + message := fmt.Sprintf("OpenBao PKI certificate request failed with HTTP %d", response.StatusCode) + if response.ContentType != "" { + message += fmt.Sprintf(" (content type %q)", response.ContentType) + } + if body := boundedOpenBaoHTTPErrorBody(response.Body); body != "" { + message += ": " + body + } + return errors.New(message) +} + +func retryablePKICertificateHTTPStatus(statusCode int) bool { + return statusCode == 0 || + statusCode == http.StatusRequestTimeout || + statusCode == http.StatusTooManyRequests || + statusCode >= http.StatusInternalServerError +} + +func boundedOpenBaoHTTPErrorBody(body string) string { + if strings.Contains(body, "-----BEGIN CERTIFICATE-----") { + return "" + } + summary := strings.Join(strings.Fields(body), " ") + if len(summary) <= maxOpenBaoHTTPErrorBody { + return summary + } + return summary[:maxOpenBaoHTTPErrorBody] + "..." +} + func waitForPKICertificateRetry(ctx context.Context, delay time.Duration) error { timer := time.NewTimer(delay) defer timer.Stop() @@ -546,6 +638,8 @@ func (c *Client) filterKubectlOutput(output string) string { lines := strings.Split(output, "\n") var jsonLines []string var pemLines []string + var plainLines []string + var httpMetadataLines []string foundJSON := false foundPEM := false @@ -557,6 +651,11 @@ func (c *Client) filterKubectlOutput(output string) string { continue } + if isCurlHTTPMetadataLine(line) { + httpMetadataLines = append(httpMetadataLines, line) + continue + } + // Handle kubectl deletion message that might be on the same line as JSON if strings.Contains(line, "pod \"") && strings.Contains(line, "deleted") { // Split at the kubectl deletion message @@ -596,23 +695,42 @@ func (c *Client) filterKubectlOutput(output string) string { if foundJSON { jsonLines = append(jsonLines, line) + } else if !foundPEM { + plainLines = append(plainLines, line) } } + var filteredOutput string if len(pemLines) > 0 { - return strings.Join(pemLines, "\n") - } - - // If no JSON found, try to return the last non-empty line - if len(jsonLines) == 0 { + filteredOutput = strings.Join(pemLines, "\n") + } else if len(jsonLines) > 0 { + filteredOutput = strings.Join(jsonLines, "\n") + } else if len(httpMetadataLines) > 0 && len(plainLines) > 0 { + filteredOutput = strings.Join(plainLines, "\n") + } else { + // If no structured response was found, return the last non-empty line. for i := len(lines) - 1; i >= 0; i-- { line := strings.TrimSpace(lines[i]) - if line != "" && !strings.Contains(line, "pod \"") && !strings.Contains(line, "deleted") { - return line + if line != "" && + !isCurlHTTPMetadataLine(line) && + !strings.Contains(line, "pod \"") && + !strings.Contains(line, "deleted") { + filteredOutput = line + break } } - return "" } - return strings.Join(jsonLines, "\n") + if len(httpMetadataLines) == 0 { + return filteredOutput + } + if filteredOutput == "" { + return strings.Join(httpMetadataLines, "\n") + } + return filteredOutput + "\n" + strings.Join(httpMetadataLines, "\n") +} + +func isCurlHTTPMetadataLine(line string) bool { + return strings.HasPrefix(line, curlHTTPStatusMarker) || + strings.HasPrefix(line, curlHTTPContentTypeMarker) } diff --git a/src/clis/nvcf-cli/internal/openbao/client_test.go b/src/clis/nvcf-cli/internal/openbao/client_test.go index 8ccd8c38a1..de33bf2428 100644 --- a/src/clis/nvcf-cli/internal/openbao/client_test.go +++ b/src/clis/nvcf-cli/internal/openbao/client_test.go @@ -21,9 +21,11 @@ import ( "context" "encoding/json" "errors" + "net/http" "os" "os/exec" "path/filepath" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -102,13 +104,13 @@ func TestRootCAPEMFromOpenBaoResponsePreservesCertificateErrors(t *testing.T) { } func TestReadPKICertificatePEMRetriesMalformedResponse(t *testing.T) { - responses := []string{ - "Internal Server Error", - `{"data":{"certificate":"-----BEGIN CERTIFICATE-----\nMIIB\n-----END CERTIFICATE-----\n"}}`, + responses := []pkiCertificateHTTPResponse{ + {StatusCode: http.StatusOK, ContentType: "text/plain", Body: "not-json"}, + {StatusCode: http.StatusOK, ContentType: "application/json", Body: `{"data":{"certificate":"-----BEGIN CERTIFICATE-----\nMIIB\n-----END CERTIFICATE-----\n"}}`}, } attempt := 0 - got, err := readPKICertificatePEM(nil, len(responses), 0, func(context.Context) (string, error) { + got, err := readPKICertificatePEM(context.Background(), len(responses), 0, func(context.Context) (pkiCertificateHTTPResponse, error) { response := responses[attempt] attempt++ return response, nil @@ -120,13 +122,13 @@ func TestReadPKICertificatePEMRetriesMalformedResponse(t *testing.T) { } func TestReadPKICertificatePEMRetriesEmptyResponse(t *testing.T) { - responses := []string{ - "", - `{"data":{"certificate":"-----BEGIN CERTIFICATE-----\nMIIB\n-----END CERTIFICATE-----\n"}}`, + responses := []pkiCertificateHTTPResponse{ + {StatusCode: http.StatusOK}, + {StatusCode: http.StatusOK, ContentType: "application/json", Body: `{"data":{"certificate":"-----BEGIN CERTIFICATE-----\nMIIB\n-----END CERTIFICATE-----\n"}}`}, } attempt := 0 - got, err := readPKICertificatePEM(context.Background(), len(responses), 0, func(context.Context) (string, error) { + got, err := readPKICertificatePEM(context.Background(), len(responses), 0, func(context.Context) (pkiCertificateHTTPResponse, error) { response := responses[attempt] attempt++ return response, nil @@ -140,15 +142,143 @@ func TestReadPKICertificatePEMRetriesEmptyResponse(t *testing.T) { func TestReadPKICertificatePEMDoesNotRetryOpenBaoError(t *testing.T) { attempt := 0 - _, err := readPKICertificatePEM(context.Background(), 3, 0, func(context.Context) (string, error) { + _, err := readPKICertificatePEM(context.Background(), 3, 0, func(context.Context) (pkiCertificateHTTPResponse, error) { attempt++ - return `{"errors":["permission denied"]}`, nil + return pkiCertificateHTTPResponse{ + StatusCode: http.StatusOK, + ContentType: "application/json", + Body: `{"errors":["permission denied"]}`, + }, nil }) require.Error(t, err) assert.Equal(t, 1, attempt) } +func TestReadPKICertificatePEMRetriesServerError(t *testing.T) { + responses := []pkiCertificateHTTPResponse{ + {StatusCode: http.StatusServiceUnavailable, ContentType: "text/plain", Body: "Internal Server Error"}, + {StatusCode: http.StatusOK, ContentType: "application/json", Body: `{"data":{"certificate":"-----BEGIN CERTIFICATE-----\nMIIB\n-----END CERTIFICATE-----\n"}}`}, + } + attempt := 0 + + got, err := readPKICertificatePEM(context.Background(), len(responses), 0, func(context.Context) (pkiCertificateHTTPResponse, error) { + response := responses[attempt] + attempt++ + return response, nil + }) + + require.NoError(t, err) + assert.Equal(t, openBaoTestCertPEM, got) + assert.Equal(t, len(responses), attempt) +} + +func TestReadPKICertificatePEMReportsServerErrorAfterRetries(t *testing.T) { + attempt := 0 + + _, err := readPKICertificatePEM(context.Background(), 3, 0, func(context.Context) (pkiCertificateHTTPResponse, error) { + attempt++ + return pkiCertificateHTTPResponse{ + StatusCode: http.StatusServiceUnavailable, + ContentType: "text/plain; charset=utf-8", + Body: "Internal Server Error", + }, nil + }) + + require.Error(t, err) + assert.Equal(t, 3, attempt) + assert.ErrorContains(t, err, "HTTP 503") + assert.ErrorContains(t, err, `content type "text/plain; charset=utf-8"`) + assert.ErrorContains(t, err, "Internal Server Error") + assert.NotContains(t, err.Error(), "invalid character") +} + +func TestReadPKICertificatePEMDoesNotRetryClientError(t *testing.T) { + attempt := 0 + + _, err := readPKICertificatePEM(context.Background(), 3, 0, func(context.Context) (pkiCertificateHTTPResponse, error) { + attempt++ + return pkiCertificateHTTPResponse{ + StatusCode: http.StatusForbidden, + ContentType: "application/json", + Body: `{"errors":["permission denied"]}`, + }, nil + }) + + require.Error(t, err) + assert.Equal(t, 1, attempt) + assert.ErrorContains(t, err, "HTTP 403") + assert.ErrorContains(t, err, "permission denied") +} + +func TestReadPKICertificatePEMReportsUnexpectedSuccessStatus(t *testing.T) { + attempt := 0 + + _, err := readPKICertificatePEM(context.Background(), 3, 0, func(context.Context) (pkiCertificateHTTPResponse, error) { + attempt++ + return pkiCertificateHTTPResponse{StatusCode: http.StatusNoContent}, nil + }) + + require.Error(t, err) + assert.Equal(t, 1, attempt) + assert.ErrorContains(t, err, "HTTP 204") +} + +func TestReadPKICertificatePEMPreservesMissingPKIError(t *testing.T) { + attempt := 0 + + _, err := readPKICertificatePEM(context.Background(), 3, 0, func(context.Context) (pkiCertificateHTTPResponse, error) { + attempt++ + return pkiCertificateHTTPResponse{ + StatusCode: http.StatusNotFound, + ContentType: "application/json", + Body: `{"errors":["no handler for route services/all/pki/root/cert/ca"]}`, + }, nil + }) + + require.Error(t, err) + assert.ErrorIs(t, err, ErrPKICertificateNotFound) + assert.Equal(t, 1, attempt) +} + +func TestPKICertificateHTTPResponseFromKubectlOutput(t *testing.T) { + c := NewClient(&Config{}, nil) + output := "Internal Server Error\nupstream unavailable\n" + + curlHTTPStatusMarker + "503\n" + + curlHTTPContentTypeMarker + "text/plain\n" + + `pod "openbao-pki-root-ca" deleted` + "\n" + + response, err := pkiCertificateHTTPResponseFromOutput(c.filterKubectlOutput(output)) + + require.NoError(t, err) + assert.Equal(t, http.StatusServiceUnavailable, response.StatusCode) + assert.Equal(t, "text/plain", response.ContentType) + assert.Equal(t, "Internal Server Error\nupstream unavailable", response.Body) +} + +func TestPKICertificateHTTPErrorBoundsResponseBody(t *testing.T) { + err := pkiCertificateHTTPError(pkiCertificateHTTPResponse{ + StatusCode: http.StatusBadGateway, + Body: strings.Repeat("x", maxOpenBaoHTTPErrorBody+100), + }) + + require.Error(t, err) + assert.Contains(t, err.Error(), strings.Repeat("x", maxOpenBaoHTTPErrorBody)) + assert.NotContains(t, err.Error(), strings.Repeat("x", maxOpenBaoHTTPErrorBody+1)) + assert.True(t, strings.HasSuffix(err.Error(), "...")) +} + +func TestPKICertificateHTTPErrorOmitsCertificateBody(t *testing.T) { + err := pkiCertificateHTTPError(pkiCertificateHTTPResponse{ + StatusCode: http.StatusBadGateway, + Body: openBaoTestCertPEM, + }) + + require.Error(t, err) + assert.ErrorContains(t, err, "") + assert.NotContains(t, err.Error(), "BEGIN CERTIFICATE") +} + func TestKubectlOutputMetadataDoesNotExposeCertificate(t *testing.T) { metadata := kubectlOutputMetadata(openBaoTestCertPEM) @@ -178,6 +308,8 @@ case " $* " in *" X-Vault-Token: "*) exit 92 ;; esac printf '%s\n' '{"data":{"certificate":"-----BEGIN CERTIFICATE-----\nMIIB\n-----END CERTIFICATE-----\n"}}' +printf '%s\n' '__NVCF_HTTP_STATUS__:200' +printf '%s\n' '__NVCF_HTTP_CONTENT_TYPE__:application/json' ` require.NoError(t, os.WriteFile(kubectlPath, []byte(kubectlScript), 0o755)) t.Setenv("PATH", testDir+string(os.PathListSeparator)+os.Getenv("PATH")) @@ -201,4 +333,5 @@ printf '%s\n' '{"data":{"certificate":"-----BEGIN CERTIFICATE-----\nMIIB\n-----E assert.NotContains(t, commands, " get secret ") assert.NotContains(t, commands, "X-Vault-Token") assert.Contains(t, commands, "/v1/services/all/pki/root/cert/ca") + assert.Contains(t, commands, "--write-out") }