Apply URL encoding/unencoding to OCSP Get requests (#18938) (#18941)

* Apply URL encoding/unencoding to OCSP Get requests

 - Missed this during development and sadly the unit tests were written
   at a level that did not expose this issue originally, there are
   certain combinations of issuer cert + serial that lead to base64
   data containing a '/' which will lead to the OCSP handler not getting
   the full parameter.
 - Do as the spec says, this should be treated as url-encoded data.

* Add cl

* Add higher level PKI OCSP GET/POST tests

* Rename PKI ocsp files to path_ocsp to follow naming conventions

* make fmt

Co-authored-by: Steven Clark <[email protected]>

builtin/logical/pki/ocsp.go → builtin/logical/pki/path_ocsp.go

@@ -13,6 +13,7 @@ import (
 	"io"
 	"math/big"
 	"net/http"
+	"net/url"
 	"time"
 
 	"github.com/hashicorp/vault/sdk/helper/errutil"
@@ -215,7 +216,12 @@ func fetchDerEncodedRequest(request *logical.Request, data *framework.FieldData)
 			return nil, errors.New("request is too large")
 		}
 
-		return base64.StdEncoding.DecodeString(base64Req)
+		unescapedBase64, err := url.QueryUnescape(base64Req)
+		if err != nil {
+			return nil, fmt.Errorf("failed to unescape base64 string: %w", err)
+		}
+
+		return base64.StdEncoding.DecodeString(unescapedBase64)
 	case logical.UpdateOperation:
 		// POST bodies should contain the binary form of the DER request.
 		// NOTE: Writing an empty update request to Vault causes a nil request.HTTPRequest, and that object

builtin/logical/pki/ocsp_test.go → builtin/logical/pki/path_ocsp_test.go

@@ -9,11 +9,15 @@ import (
 	"fmt"
 	"io"
 	"net/http"
+	"net/url"
 	"strconv"
 	"strings"
 	"testing"
 	"time"
 
+	vaulthttp "github.com/hashicorp/vault/http"
+	"github.com/hashicorp/vault/vault"
+
 	"github.com/hashicorp/vault/sdk/logical"
 	"github.com/stretchr/testify/require"
 	"golang.org/x/crypto/ocsp"
@@ -362,6 +366,102 @@ func TestOcsp_MultipleMatchingIssuersOneWithoutSigningUsage(t *testing.T) {
 	requireOcspResponseSignedBy(t, ocspResp, rotatedCert)
 }
 
+// Make sure OCSP GET/POST requests work through the entire stack, and not just
+// through the quicker backend layer the other tests are doing.
+func TestOcsp_HigherLevel(t *testing.T) {
+	t.Parallel()
+	coreConfig := &vault.CoreConfig{
+		LogicalBackends: map[string]logical.Factory{
+			"pki": Factory,
+		},
+	}
+	cluster := vault.NewTestCluster(t, coreConfig, &vault.TestClusterOptions{
+		HandlerFunc: vaulthttp.Handler,
+	})
+	cluster.Start()
+	defer cluster.Cleanup()
+	client := cluster.Cores[0].Client
+	mountPKIEndpoint(t, client, "pki")
+	resp, err := client.Logical().Write("pki/root/generate/internal", map[string]interface{}{
+		"key_type":    "ec",
+		"common_name": "root-ca.com",
+		"ttl":         "600h",
+	})
+
+	require.NoError(t, err, "error generating root ca: %v", err)
+	require.NotNil(t, resp, "expected ca info from root")
+
+	issuerCert := parseCert(t, resp.Data["certificate"].(string))
+
+	resp, err = client.Logical().Write("pki/roles/example", map[string]interface{}{
+		"allowed_domains":  "example.com",
+		"allow_subdomains": "true",
+		"no_store":         "false", // make sure we store this cert
+		"max_ttl":          "1h",
+		"key_type":         "ec",
+	})
+	require.NoError(t, err, "error setting up pki role: %v", err)
+
+	resp, err = client.Logical().Write("pki/issue/example", map[string]interface{}{
+		"common_name": "test.example.com",
+		"ttl":         "15m",
+	})
+	require.NoError(t, err, "error issuing certificate: %v", err)
+	require.NotNil(t, resp, "got nil response from issuing request")
+	certToRevoke := parseCert(t, resp.Data["certificate"].(string))
+	serialNum := resp.Data["serial_number"].(string)
+
+	// Revoke the certificate
+	resp, err = client.Logical().Write("pki/revoke", map[string]interface{}{
+		"serial_number": serialNum,
+	})
+	require.NoError(t, err, "error revoking certificate: %v", err)
+	require.NotNil(t, resp, "got nil response from revoke")
+
+	// Make sure that OCSP handler responds properly
+	ocspReq := generateRequest(t, crypto.SHA256, certToRevoke, issuerCert)
+	ocspPostReq := client.NewRequest(http.MethodPost, "/v1/pki/ocsp")
+	ocspPostReq.Headers.Set("Content-Type", "application/ocsp-request")
+	ocspPostReq.BodyBytes = ocspReq
+	rawResp, err := client.RawRequest(ocspPostReq)
+	require.NoError(t, err, "failed sending ocsp post request")
+
+	require.Equal(t, 200, rawResp.StatusCode)
+	require.Equal(t, ocspResponseContentType, rawResp.Header.Get("Content-Type"))
+	bodyReader := rawResp.Body
+	respDer, err := io.ReadAll(bodyReader)
+	bodyReader.Close()
+	require.NoError(t, err, "failed reading response body")
+
+	ocspResp, err := ocsp.ParseResponse(respDer, issuerCert)
+	require.NoError(t, err, "parsing ocsp get response")
+
+	require.Equal(t, ocsp.Revoked, ocspResp.Status)
+	require.Equal(t, issuerCert, ocspResp.Certificate)
+	require.Equal(t, certToRevoke.SerialNumber, ocspResp.SerialNumber)
+
+	// Test OCSP Get request for ocsp
+	urlEncoded := url.QueryEscape(base64.StdEncoding.EncodeToString(ocspReq))
+	ocspGetReq := client.NewRequest(http.MethodGet, "/v1/pki/ocsp/"+urlEncoded)
+	ocspGetReq.Headers.Set("Content-Type", "application/ocsp-request")
+	rawResp, err = client.RawRequest(ocspGetReq)
+	require.NoError(t, err, "failed sending ocsp get request")
+
+	require.Equal(t, 200, rawResp.StatusCode)
+	require.Equal(t, ocspResponseContentType, rawResp.Header.Get("Content-Type"))
+	bodyReader = rawResp.Body
+	respDer, err = io.ReadAll(bodyReader)
+	bodyReader.Close()
+	require.NoError(t, err, "failed reading response body")
+
+	ocspResp, err = ocsp.ParseResponse(respDer, issuerCert)
+	require.NoError(t, err, "parsing ocsp get response")
+
+	require.Equal(t, ocsp.Revoked, ocspResp.Status)
+	require.Equal(t, issuerCert, ocspResp.Certificate)
+	require.Equal(t, certToRevoke.SerialNumber, ocspResp.SerialNumber)
+}
+
 func TestOcsp_ValidRequests(t *testing.T) {
 	type caKeyConf struct {
 		keyType string
@@ -584,7 +684,7 @@ func sendOcspRequest(t *testing.T, b *backend, s logical.Storage, getOrPost stri
 }
 
 func sendOcspGetRequest(b *backend, s logical.Storage, ocspRequest []byte) (*logical.Response, error) {
-	urlEncoded := base64.StdEncoding.EncodeToString(ocspRequest)
+	urlEncoded := url.QueryEscape(base64.StdEncoding.EncodeToString(ocspRequest))
 	return CBRead(b, s, "ocsp/"+urlEncoded)
 }
 

changelog/18938.txt

@@ -0,0 +1,3 @@
+```release-note:bug
+secrets/pki: OCSP GET request parameter was not being URL unescaped before processing.
+```