Skip to content

Cache update #6

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.

Sign up for GitHub

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

Open
mlech-reef wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Cache update #6

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8695959
Move handling cache to authHandler
mlech-reef Oct 24, 2025
191e141
Change default cache duration to 12min, remove upper limit for clean …
mlech-reef Oct 24, 2025
e71e56a
Try to get pubkey from pylon in case of the cached one doesn't match
mlech-reef Oct 24, 2025
ddd7266
Move org name sanitization to the extractOrganizationName
mlech-reef Oct 31, 2025
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
141 changes: 77 additions & 64 deletions internal/auth/auth.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,71 +146,99 @@ func (a *Auth) authHandler(w http.ResponseWriter, r *http.Request) {
clientCertHeader := r.Header.Get("X-Client-Cert")
if clientCertHeader == "" {
log.Printf("No X-Client-Cert header found")
w.WriteHeader(http.StatusForbidden)
if _, err := w.Write([]byte("Access denied: No client certificate")); err != nil {
log.Printf("failed to write response body: %v", err)
}
a.writeForbidden(w, "Access denied: No client certificate")
return
}

// URL decode the certificate (nginx typically URL-encodes it)
decodedCert, err := url.QueryUnescape(clientCertHeader)
if err != nil {
log.Printf("Failed to URL decode certificate: %v", err)
w.WriteHeader(http.StatusForbidden)
if _, err := w.Write([]byte("Access denied: Invalid certificate encoding")); err != nil {
log.Printf("failed to write response body: %v", err)
}
a.writeForbidden(w, "Access denied: Invalid certificate encoding")
return
}

// Parse the certificate
cert, err := a.parseCertificate(decodedCert)
if err != nil {
log.Printf("Failed to parse certificate: %v", err)
w.WriteHeader(http.StatusForbidden)
if _, err := w.Write([]byte("Access denied: Invalid certificate")); err != nil {
log.Printf("failed to write response body: %v", err)
}
a.writeForbidden(w, "Access denied: Invalid certificate")
return
}

// Extract Organization Name (O) from certificate
orgName := a.extractOrganizationName(cert)
if orgName == "" {
log.Printf("No organization name found in certificate")
w.WriteHeader(http.StatusForbidden)
if _, err := w.Write([]byte("Access denied: No organization in certificate")); err != nil {
log.Printf("failed to write response body: %v", err)
}
return
}

// Load the expected public key from Pylon using the organization name as hotkey
expectedPub, err := a.loadExpectedPublicKey(orgName)
// Extract and sanitize Organization Name (O) from certificate
sanitizedOrgName, err := a.extractOrganizationName(cert)
if err != nil {
log.Printf("Failed to load expected public key for organization '%s': %v", orgName, err)
w.WriteHeader(http.StatusForbidden)
if _, err := w.Write([]byte("Access denied: Organization not authorized")); err != nil {
log.Printf("failed to write response body: %v", err)
}
log.Printf("Failed to extract organization name: %v", err)
a.writeForbidden(w, "Access denied: Invalid organization in certificate")
return
}

// Validate the certificate against the expected public key
if err := a.validatePublicKey(cert, expectedPub); err != nil {
log.Printf("Certificate validation failed for organization '%s': %v", orgName, err)
w.WriteHeader(http.StatusForbidden)
if _, err := w.Write([]byte("Access denied: Certificate validation failed")); err != nil {
log.Printf("failed to write response body: %v", err)
// Check cache first
if cachedKey, found := a.cache.Get(sanitizedOrgName); found {
log.Printf("Cache hit for organization '%s'", sanitizedOrgName)

Copy link
Copy Markdown

@jakub-zytka-reef jakub-zytka-reef Oct 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I find this code difficult to follow.

I think this would be better:

if cachedKey, found := a.cache.Get(sanitizedOrgName); found {
		log cache hit
		if err := a.validatePublicKey(cert, cachedKey); err == nil {
		   log auth granted
		   w.writeOK("Access granted ...)
		   return
		} else {
           log invalid cache
           a.cache.Invalidate(sanitizedOrgName)
        }
}

expectedPub, err := a.loadExpectedPublicKey(sanitizedOrgName)
if err != nil {
				log.Printf("Failed to load expected public key for organization '%s': %v", sanitizedOrgName, err)
				a.writeForbidden(w, "Access denied: Organization not authorized")
				return
}

// Store fresh key in cache
a.cache.Set(sanitizedOrgName, expectedPub)

if err := a.validatePublicKey(cert, expectedPub); err != nil {
				log.Printf("Certificate validation failed for organization '%s': %v", orgName, err)
				a.writeForbidden(w, "Access denied: Certificate validation failed")
				return
} else {
    log.Printf("Certificate validation successful for organization '%s'", orgName)
	a.writeOK(w, "Access granted")
}

notice how much shorter the code is, and how there's almost no nested ifs. Also the context doesn't jump around - in the original code when the cached key is correctly validated you first need to jump 20 lines from if err := a.validatePublicKey(cert, cachedKey); err != nil to the end of the if, then ~25 lines again down to see what happens when you leave the big if
You loose some context in the log lines (but not in the logs in their entirety), but get much simpler code. I take this tradeoff 10 out of 10 times.

Btw, there is tight coupling of auth logic with response writing, that manifests by

a.writeForbidden(...)
return

were the code written like

return CERTIFICATE_VALIDATION_FAILED

and the caller would use the writer to actually write the decision you could test each aspect separately, and the code wouldn't need much changes if there was another writer used in the future. Now, the logic of deciding whether a cert is good relies on http.ResponseWriter. There is no need for that.

Copy link
Copy Markdown
Contributor Author

@mlech-reef mlech-reef Oct 31, 2025

Choose a reason for hiding this comment

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

I kindly disagree.

Current code is much more readable:

  1. Happy path only ends in one place - at the end of the file. In nested places, you return only in case of unhappy path
  2. You have clear separation of two main paths: (a) cache hit in if, (b) cache miss in else. So quick look at the code and you immediately know what is going on. I don't mind have some code duplication, if I can have better logs (maybe it is devops inside me)
  3. This is not large if...else. Most of the lines are error handling via values, which is go thing and you simply must get used to that

I am happy to discuss it in more details ;)

Copy link
Copy Markdown

@jakub-zytka-reef jakub-zytka-reef Nov 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Happy path only ends in one place - at the end of the file. In nested places, you return only in case of unhappy path

This is one of the reasons why the code is needlessly complex.
There isn't one happy path. There are multiple:

  1. cache hit, certificate valid
  2. cache hit, certificate invalid, fresh key valid
  3. cache miss, fresh key valid

They were made convoluted so that they end up together in one line.

So let's consider the complexity of happy paths. I want to check if we don't OK in some case that we shouldn't. That's a reasonable ask, isn't it?

Since I read the code from top to bottom, I don't know you made the decision to return OK as the very last thing in the function.

And so to achieve my goal I need to go through the whole 50 lines of code back and forth and check every set of possible conditions to see what is happening there.

So it goes like this:
read lines 178-181. I need to push "cache was hit" to the context.
then I read 182-184. I need to push "public key not validated" to the context, to make it "cache was hit, public key not validated"
then I read 188-202. I push and pop from the context because there are ifs.
I need to remember that if a set of 3 conditions evaluates to some specific values the access will be forbidden.
In 203/204 I need to scan back to 178 to check what the closing bracket are about. I could pop from the context stack, but by that time I read 20 lines of code, made 4 operations on the context, and have to remember 2 specific cases in which 3 ifs evaluate to specific values, and that then the access is forbidden. That's a lot, so I don't want to make a mistake. I scan back, refresh my memory on the context.
So far I scanned ~50 lines of code already.

in 204 there's an else. I scan down, because for the time being I don't care for happens in the other branch. I could go there, but then the amount of information that I need to remember grows even larger.
So I scan to 226 to learn that now the access is granted.
This means that the if's I read earlier must exhaust all the cases where the access should be forbidden. Does it? I need to scan back to 183-202 part and find the answer to a different question than before:
"do, under the conditions from line 178 and 182 conditions from 189 and 198 exhaust all the possibilities where we should deny access".

I've scanned the same code multiple times and I haven't even touched the else block.

Let's compare that to my proposal:
it has a flat structure, each part is independent. There is no reason to scan back in any place.
The first part:

if cachedKey, found := a.cache.Get(sanitizedOrgName); found {
		log cache hit
		if err := a.validatePublicKey(cert, cachedKey); err == nil {
		   log auth granted
		   w.writeOK("Access granted ...)
		   return
		} else {
           log invalid cache
           a.cache.Invalidate(sanitizedOrgName)
        }
}

has ~10 lines and simple semantics:
if there's a correct key in cache - allow access; if not the cache will not have a key.

After reading that part and deeming that semantics is satisfied you can forget about it's contents entirely. The ONLY thing you need to remember is that "if the control progressed after the block then there is no key in the cache"; all scenarios with the key in the cache have been dealt with (correctly).
So far we dealt with "cache hit" scenario. There were ~10 lines and 2 ifs in total.

The second part is also dead simple - load the key to the cache. If it cannot be loaded forbid access:

expectedPub, err := a.loadExpectedPublicKey(sanitizedOrgName)
if err != nil {
				log.Printf("Failed to load expected public key for organization '%s': %v", sanitizedOrgName, err)
				a.writeForbidden(w, "Access denied: Organization not authorized")
				return
}

// Store fresh key in cache
a.cache.Set(sanitizedOrgName, expectedPub)

Again, once you are satisfied this semantics is achieved you can forget about the details of that part. With 1 if you reach a point in code where "there is a fresh public key in the cache". All other scenarios have been dealt with correctly, so no need to concern yourself with that.

And so there's the final part:

if err := a.validatePublicKey(cert, expectedPub); err != nil {
				log.Printf("Certificate validation failed for organization '%s': %v", orgName, err)
				a.writeForbidden(w, "Access denied: Certificate validation failed")
				return
} else {
    log.Printf("Certificate validation successful for organization '%s'", orgName)
	a.writeOK(w, "Access granted")
}

Again, it's dead simple and you can tell the "fresh key in the cache" is handled correctly.
There were 4 ifs in total, the max nesting was 2. You never had to have more than ~10 lines of code in the context + 1 line uncoditional state description. Regardless of whether you were looking for happy paths or negative paths you had to do the same amount of work or the order of function length.

In your solution there are 6 ifs, with max nesting 3. It is necessary to keep much more in the context (say, ~20 lines + scanning over lines that are not interesting), keeping an intricate state descriptions like the cache was hit, the key was invalid, it was unable to load key or it was possible to load the key but they key was not valid.

I encourage you to measure code complexity of the proposed solutions using existing tools...

You have clear separation of two main paths: (a) cache hit in if, (b) cache miss in else. So quick look at the code and you immediately know what is going on. I don't mind have some code duplication, if I can have better logs (maybe it is devops inside me)

This forces you have a state that is a cross product of different conditions, making the code more complex and more difficult to test, as the conditions are independent (e.g. the key is valid or not regardless of if it was loaded from cache or subtensor).
You can have equally good logs in my solution, but since it's linear you might need to read more than 1 line of log. This is a very good tradeoff, because simpler code means less bugs means less log reading in the first place. Reading 2 lines instead of 1 is near zero cost once you actually start reading the logs.

This is not large if...else.

It is needlessly larger than it needs to be by every metric I know of.
That said, I'm open to any new perspective.

Most of the lines are error handling via values, which is go thing and you simply must get used to that

this is completely irrelevant.
FWIW, you handled errors by writing to a http response rather than by returning values, which introduces unnecessary coupling to http, but OK, let's keep the changes to minimum and let's hope there won't be any other use for the auth mechanism and so we won't need to refactor that in the future.

Copy link
Copy Markdown
Contributor Author

@mlech-reef mlech-reef Nov 5, 2025

Choose a reason for hiding this comment

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

I really don't want to delve into this discussion. Clearly, we have different opinion. Just 2 things I want to share.

  1. More code != more complexity. And I am not talking about cyclomatic complexity, like mccabe which is mostly useless. I m talking about developer experience. As a experienced developer (I think), I like my code better than yours. I would prefer to read my code than yours. I am more than sure, I wold understand my code faster than yours. You haven't gave me any solid arguments that I should change my mind, other than some subjective opinions. So even if prefer your "style", I would probably keep this one as the logging is better - which is a way way more important IMHO than some theoretical divagation about how developers read the code.
  2. Coupling to http is obvious as this is http handler with predefined (by the go standard library) function signature. Abstracting prematurely is a cancer caused by the few influencing books (Clean Code and others). Maybe it works for Java, I don't know.

So please close this PR or like described in the handbook:

If there is a change that is up to taste, the reviewer should refrain from suggesting it. If there is no concrete gain from the change (and maybe others prefer the way of the author, so we don't really know which is better), then it's better not to spend the energy on a discussion and just accept the code as is. We call it “the author's preference”.

The author of the PR must take all review comments into consideration, but as a senior engineer, they are expected to make a decision on how to address them.

let's merge it if you don't see any bugs and you can change it in any way you want after that, signing the commits with your name ;)

No hard feelings from my side. I love heat discussions about coding :) Hopefully you feel the same way,

// Validate with cached key
if err := a.validatePublicKey(cert, cachedKey); err != nil {
// Validation failed with cached key - invalidate cache and retry with fresh data
log.Printf("Certificate validation failed with cached key for organization '%s', invalidating cache and retrying", sanitizedOrgName)
a.cache.Invalidate(sanitizedOrgName)

// Fetch fresh public key from Pylon
expectedPub, err := a.loadExpectedPublicKey(sanitizedOrgName)
if err != nil {
log.Printf("Failed to load expected public key for organization '%s' on retry: %v", sanitizedOrgName, err)
a.writeForbidden(w, "Access denied: Organization not authorized")
return
}
// Store fresh key in cache
a.cache.Set(sanitizedOrgName, expectedPub)

// Validate again with fresh key
if err := a.validatePublicKey(cert, expectedPub); err != nil {
log.Printf("Certificate validation failed for organization '%s' even after cache refresh: %v", sanitizedOrgName, err)
a.writeForbidden(w, "Access denied: Certificate validation failed")
return
}
}
} else {
log.Printf("Cache miss for organization '%s'", sanitizedOrgName)

// Cache miss - load from Pylon
expectedPub, err := a.loadExpectedPublicKey(sanitizedOrgName)
if err != nil {
log.Printf("Failed to load expected public key for organization '%s': %v", sanitizedOrgName, err)
a.writeForbidden(w, "Access denied: Organization not authorized")
return
}
// Store in cache
a.cache.Set(sanitizedOrgName, expectedPub)

// Validate with freshly loaded key
if err := a.validatePublicKey(cert, expectedPub); err != nil {
log.Printf("Certificate validation failed for organization '%s': %v", sanitizedOrgName, err)
a.writeForbidden(w, "Access denied: Certificate validation failed")
return
}
return
}

// Certificate is valid
log.Printf("Certificate validation successful for organization '%s'", orgName)
log.Printf("Certificate validation successful for organization '%s'", sanitizedOrgName)
a.writeOK(w, "Access granted")
}

// writeForbidden writes a 403 Forbidden response with the given message
func (a *Auth) writeForbidden(w http.ResponseWriter, message string) {
w.WriteHeader(http.StatusForbidden)
if _, err := w.Write([]byte(message)); err != nil {
log.Printf("failed to write response body: %v", err)
}
}

// writeOK writes a 200 OK response with the given message
func (a *Auth) writeOK(w http.ResponseWriter, message string) {
w.WriteHeader(http.StatusOK)
if _, err := w.Write([]byte("Access granted")); err != nil {
if _, err := w.Write([]byte(message)); err != nil {
log.Printf("failed to write response body: %v", err)
}
}
Expand All @@ -235,12 +263,14 @@ func (a *Auth) parseCertificate(certPEM string) (*x509.Certificate, error) {
return cert, nil
}

// extractOrganizationName extracts the Organization Name (O) from the certificate
func (a *Auth) extractOrganizationName(cert *x509.Certificate) string {
if len(cert.Subject.Organization) > 0 {
return cert.Subject.Organization[0]
// extractOrganizationName extracts and sanitizes the Organization Name (O) from the certificate
func (a *Auth) extractOrganizationName(cert *x509.Certificate) (string, error) {
if len(cert.Subject.Organization) == 0 {
return "", fmt.Errorf("no organization name found in certificate")
}
return ""

orgName := cert.Subject.Organization[0]
return a.sanitizeOrgName(orgName)
}

// sanitizeOrgName sanitizes the organization name to prevent path traversal attacks
Expand Down Expand Up @@ -273,24 +303,10 @@ func (a *Auth) sanitizeOrgName(orgName string) (string, error) {
}

// loadExpectedPublicKey fetches expected ed25519 public key for given organization (hotkey) from Pylon
// Uses cache to avoid repeated requests for the same hotkey
// Note: orgName should already be sanitized by the caller
Copy link
Copy Markdown

@jakub-zytka-reef jakub-zytka-reef Oct 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think we could enforce this instead of adding a comment. (Yes, I'm a type system junkie. No, I'm not well versed in golang, so please bear with me...)

Let the orgName be of type SanitizedOrgName, and the only producer of said type be sanitizeOrgName
Or actually, the type constructor should sanitize and that's all.

WDYT?

Copy link
Copy Markdown
Contributor Author

@mlech-reef mlech-reef Oct 31, 2025

Choose a reason for hiding this comment

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

IMHO I don't want to create another abstraction and custom type for something that is just a string and is purely internal in auth.go and not publicly available outside. Smells like Java/OOP

However, to address your concern and still satisfy procedural go code, I can just move sanitization to the extractOrganizationName and the problem is solved

Copy link
Copy Markdown

@jakub-zytka-reef jakub-zytka-reef Nov 5, 2025

Choose a reason for hiding this comment

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

I'm eager to learn.

Please tell me, what in type SanitzedOrgName string smells you of Java, and how does this construct relates to OOP.

Copy link
Copy Markdown
Contributor Author

@mlech-reef mlech-reef Nov 5, 2025

Choose a reason for hiding this comment

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

This is not, other than it is really redundant IMHO. I was more referring to creating another struct with a new New method, just to be able to sanitize it there

func (a *Auth) loadExpectedPublicKey(orgName string) (ed25519.PublicKey, error) {
// Sanitize the organization name to prevent path traversal attacks
sanitizedOrgName, err := a.sanitizeOrgName(orgName)
if err != nil {
return nil, fmt.Errorf("invalid organization name: %v", err)
}

// Check cache first
if cachedKey, found := a.cache.Get(sanitizedOrgName); found {
log.Printf("Cache hit for organization '%s'", sanitizedOrgName)
return cachedKey, nil
}

log.Printf("Cache miss for organization '%s'", sanitizedOrgName)

// Cache miss - fetch from Pylon
resp, err := a.pylonClient.GetCertificate(sanitizedOrgName)
// Fetch from Pylon
resp, err := a.pylonClient.GetCertificate(orgName)
if err != nil {
return nil, err
}
Expand All @@ -307,9 +323,6 @@ func (a *Auth) loadExpectedPublicKey(orgName string) (ed25519.PublicKey, error)
}
publicKey := ed25519.PublicKey(decoded)

// Store in cache
a.cache.Set(sanitizedOrgName, publicKey)

return publicKey, nil
}

Expand Down
159 changes: 140 additions & 19 deletions internal/auth/auth_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,29 +99,34 @@ func TestExtractOrganizationName(t *testing.T) {
auth := &Auth{}

tests := []struct {
name string
orgNames []string
expected string
name string
orgNames []string
expected string
expectError bool
}{
{
name: "single organization",
orgNames: []string{"Test Organization"},
expected: "Test Organization",
name: "single organization",
orgNames: []string{"Test Organization"},
expected: "Test Organization",
expectError: false,
},
{
name: "multiple organizations",
orgNames: []string{"First Org", "Second Org"},
expected: "First Org",
name: "multiple organizations",
orgNames: []string{"First Org", "Second Org"},
expected: "First Org",
expectError: false,
},
{
name: "no organizations",
orgNames: []string{},
expected: "",
name: "no organizations",
orgNames: []string{},
expected: "",
expectError: true,
},
{
name: "nil organizations",
orgNames: nil,
expected: "",
name: "nil organizations",
orgNames: nil,
expected: "",
expectError: true,
},
}

Expand All @@ -133,9 +138,18 @@ func TestExtractOrganizationName(t *testing.T) {
},
}

result := auth.extractOrganizationName(cert)
if result != tt.expected {
t.Errorf("Expected %q, got %q", tt.expected, result)
result, err := auth.extractOrganizationName(cert)
if tt.expectError {
if err == nil {
t.Errorf("Expected error but got nil")
}
} else {
if err != nil {
t.Errorf("Unexpected error: %v", err)
}
if result != tt.expected {
t.Errorf("Expected %q, got %q", tt.expected, result)
}
}
})
}
Expand Down Expand Up @@ -193,7 +207,6 @@ func TestLoadExpectedPublicKey(t *testing.T) {
{name: "missing key", orgName: "missing", expectError: true},
{name: "wrong algorithm", orgName: "wrongalgo", expectError: true},
{name: "not found", orgName: "unknown", expectError: true},
{name: "empty", orgName: "", expectError: true},
}

for _, tt := range tests {
Expand Down Expand Up @@ -487,6 +500,114 @@ func TestAuthHandler(t *testing.T) {
}
}

// TestAuthHandler_CacheInvalidationRetry tests that when validation fails with a cached key,
// the cache is invalidated and a fresh key is fetched from Pylon
func TestAuthHandler_CacheInvalidationRetry(t *testing.T) {
// Create two different key pairs
oldPub, _, _ := ed25519.GenerateKey(rand.Reader)
newPub, newPriv, _ := ed25519.GenerateKey(rand.Reader)

// Create a certificate with the NEW key
testCert := createTestCertificateWithKey(t, "Test Organization", newPriv)

newPubHex := hex.EncodeToString(newPub)

// Track how many times Pylon is called
callCount := 0

// Mock Pylon server that returns the new (correct) key
// The old key is already in the cache, so Pylon will only be called after invalidation
ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
if strings.HasSuffix(r.URL.Path, "/Test Organization") {
callCount++
w.Header().Set("Content-Type", "application/json")
w.WriteHeader(http.StatusOK)
// Always return the new (correct) key when Pylon is called
_, _ = fmt.Fprintf(w, `{"public_key":"%s","algorithm":1}`, newPubHex)
return
}
w.WriteHeader(http.StatusNotFound)
}))
defer ts.Close()

config := &configuration.Config{
PylonEndpoint: ts.URL + "/",
CacheDurationMins: 12, // Enable caching
}
auth := NewAuth(config)

// Pre-populate cache with OLD key (using sanitized name)
sanitizedOrgName, _ := auth.sanitizeOrgName("Test Organization")
auth.cache.Set(sanitizedOrgName, oldPub)

// Make request with certificate that has NEW key
req := httptest.NewRequest("GET", "/", nil)
req.Header.Set("X-Client-Cert", url.QueryEscape(encodeCertificateToPEM(testCert)))

rr := httptest.NewRecorder()
auth.authHandler(rr, req)

// Should succeed because:
// 1. First validation fails with cached old key
// 2. Cache is invalidated
// 3. Fresh key is fetched from Pylon (new key)
// 4. Second validation succeeds with new key
if rr.Code != http.StatusOK {
t.Errorf("Expected status %d, got %d", http.StatusOK, rr.Code)
}

// Pylon should have been called once (to get the fresh key after cache invalidation)
if callCount != 1 {
t.Errorf("Expected Pylon to be called 1 time, got %d", callCount)
}
}

// TestAuthHandler_CacheInvalidationRetryStillFails tests that when validation fails even after retry,
// the request is still denied
func TestAuthHandler_CacheInvalidationRetryStillFails(t *testing.T) {
// Create two different key pairs - certificate has one, Pylon returns another (both times)
wrongPub, _, _ := ed25519.GenerateKey(rand.Reader)
_, certPriv, _ := ed25519.GenerateKey(rand.Reader)

testCert := createTestCertificateWithKey(t, "Test Organization", certPriv)

wrongPubHex := hex.EncodeToString(wrongPub)

// Mock Pylon server that always returns the wrong key
ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
if strings.HasSuffix(r.URL.Path, "/Test Organization") {
w.Header().Set("Content-Type", "application/json")
w.WriteHeader(http.StatusOK)
_, _ = fmt.Fprintf(w, `{"public_key":"%s","algorithm":1}`, wrongPubHex)
return
}
w.WriteHeader(http.StatusNotFound)
}))
defer ts.Close()

config := &configuration.Config{
PylonEndpoint: ts.URL + "/",
CacheDurationMins: 12, // Enable caching
}
auth := NewAuth(config)

// Pre-populate cache with wrong key (using sanitized name)
sanitizedOrgName, _ := auth.sanitizeOrgName("Test Organization")
auth.cache.Set(sanitizedOrgName, wrongPub)

// Make request
req := httptest.NewRequest("GET", "/", nil)
req.Header.Set("X-Client-Cert", url.QueryEscape(encodeCertificateToPEM(testCert)))

rr := httptest.NewRecorder()
auth.authHandler(rr, req)

// Should fail because even after cache refresh, the key still doesn't match
if rr.Code != http.StatusForbidden {
t.Errorf("Expected status %d, got %d", http.StatusForbidden, rr.Code)
}
}

// Helper functions for testing

func createTestCertificate(t *testing.T, orgName string) *x509.Certificate {
Expand Down
Loading
Loading