Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 13 additions & 5 deletions server/certidp/ocsp_responder.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
"fmt"
"io"
"net/http"
"net/url"
"strings"
"time"

Expand Down Expand Up @@ -55,7 +56,7 @@ func FetchOCSPResponse(link *ChainLink, opts *OCSPPeerConfig, log *Log) ([]byte,
return nil, err
}

reqEnc := base64.StdEncoding.EncodeToString(reqDER)
reqEnc := encodeOCSPRequest(reqDER)

responders := *link.OCSPWebEndpoints

Expand All @@ -68,10 +69,10 @@ func FetchOCSPResponse(link *ChainLink, opts *OCSPPeerConfig, log *Log) ([]byte,
Timeout: timeout,
}
for _, u := range responders {
url := u.String()
log.Debugf(DbgMakingCARequest, url)
url = strings.TrimSuffix(url, "/")
raw, err = getRequestBytes(fmt.Sprintf("%s/%s", url, reqEnc), hc)
responderURL := u.String()
log.Debugf(DbgMakingCARequest, responderURL)
responderURL = strings.TrimSuffix(responderURL, "/")
raw, err = getRequestBytes(fmt.Sprintf("%s/%s", responderURL, reqEnc), hc)
if err == nil {
break
}
Expand All @@ -82,3 +83,10 @@ func FetchOCSPResponse(link *ChainLink, opts *OCSPPeerConfig, log *Log) ([]byte,

return raw, nil
}

// encodeOCSPRequest encodes the OCSP request in base64 and URL-encodes it.
// This is needed to fulfill the OCSP responder's requirements for the request format. (X.690)
func encodeOCSPRequest(reqDER []byte) string {
reqEnc := base64.StdEncoding.EncodeToString(reqDER)
Copy link
Member

Choose a reason for hiding this comment

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

Should this be base64.RawURLEncoding.EncodeToString(...) and then you can get rid of url.QueryEscape?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was hesitant to remove the padding characters because I don't know how the decoder is implemented. It should be able to determine the correct size by measuring and rounding up, though.

However, using the URLEncoding functions definitely makes sense.

Copy link
Member

Choose a reason for hiding this comment

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

Good point about the padding, in which case URLEncoding should be fine indeed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will test this against our PKI first and report back.

Copy link
Member

Choose a reason for hiding this comment

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

How did it go?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not well. The PKI we use can't handle some of the options, but I'm still waiting for a response.

I also checked back with the RFC 4648 and the URLEncode is not the same like normal base64.

See https://datatracker.ietf.org/doc/html/rfc4648#section-5 for reference.

Right now I think that my first approach of using the StdEncoding (without removing padding characters) and then url.Encoding is safer. Will update this PR as soon as I get feedback from my colleagues.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I've tested all the combinations against our PKI system.

Only the 'normal' Base64 encoding, including the padding characters, worked.

Therefore, I would stick with my initial proposal to fix the bug and achieve the greatest possible compatibility with external PKI systems.

return url.QueryEscape(reqEnc)
}
29 changes: 29 additions & 0 deletions server/certidp/ocsp_responder_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
package certidp

import (
"encoding/base64"
"net/url"
"strings"
"testing"
)

func TestEncodeOCSPRequest(t *testing.T) {
data := []byte("test data for OCSP request")
encoded := encodeOCSPRequest(data)

if strings.ContainsAny(encoded, "+/=") {
t.Errorf("URL contains unescaped characters: %s", encoded)
}

decodedURL, err := url.QueryUnescape(encoded)
if err != nil {
t.Errorf("failed to url-decode request: %v", err)
}
decodedData, err := base64.StdEncoding.DecodeString(decodedURL)
if err != nil {
t.Errorf("failed to base64-decode request: %v", err)
}
if string(decodedData) != string(data) {
t.Errorf("Decoded data does not match original: got %s, want %s", decodedData, data)
}
}