-
Notifications
You must be signed in to change notification settings - Fork 420
Fix #481: Add server name scoping support in DNS TXT records #490
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
base: main
Are you sure you want to change the base?
Fix #481: Add server name scoping support in DNS TXT records #490
Conversation
internal/api/handlers/v0/auth/dns.go
Outdated
var validRecord *DNSAuthRecord | ||
for i, record := range authRecords { | ||
if ed25519.Verify(record.PublicKey, messageBytes, signature) { | ||
validRecord = &authRecords[i] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the order of the TXT records is undefined and this is given "[random?] first wins". What about collecting all of the valid records an expressing the union of the permissions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joelverhagen Great catch! I've updated the implementation to collect ALL valid records and express the union of permissions, exactly as you suggested.
{ | ||
Action: auth.PermissionActionPublish, | ||
ResourcePattern: fmt.Sprintf("%s.*", reverseDomain), | ||
ResourcePattern: namePattern, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This allows arbitrary access, I think, by putting say n= *
in the record. This would give the *
name space, which is a severe security issue, if I'm not mistaken.
I believe we must include the reverseDomain
in the resource pattern otherwise we will allow access to multiple domains.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively, we can check that namePattern
starts with reverseDomain
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joelverhagen You're absolutely right, allowing arbitrary patterns would be a severe vulnerability.
I've implemented your suggested solution
expectedNumPerms: 2, | ||
expectError: false, | ||
}, | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should have some tests where the domain is not present in the name pattern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joelverhagen test cases added.
f31ec95
to
8791079
Compare
squashed and pushed |
DNS_SCOPING_TEST_GUIDE.md
Outdated
@@ -0,0 +1,294 @@ | |||
# DNS TXT Name Scoping - Testing Guide |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if this should be in the root of the repo. If we scale this approach out, the root will be filled with a lot of rather specific test guides. I do like the idea of documenting the tests for human and machine alike.
Maybe under tests
or docs/tests
or tests/docs
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joelverhagen I will remove
fmt.Sprintf("v=MCPv1; k=ed25519; p=%s; n=com.example/services/auth/*", publicKeyB64_1), | ||
}, | ||
usePrivateKey: privateKey1, | ||
expectedPatterns: []string{"com.example/services/auth/*"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With #476 this name is no longer valid (exactly 1 slash per server name). It seems okay to test this since server name validation is done elsewhere) but IMHO we should reject bad server name patterns as early as possible. No sure what other people thing though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can modify the test case. It will now expect an error when an invalid name pattern is used. This way, if the validation logic ever changes or is accidentally removed, the test will fail and immediately alert us to the regression.
Thoughts?
expectError: true, | ||
errorContains: "no valid permissions found", | ||
}, | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we have tests for a SLD prefixing another? I am looking at the code and, with the meager caffeine fix I have in me right now, I can't see if there's a problem. But I think so.
Suppose the domain is micro.com
and the n=com.microsoft/*
, I think this would be allowed by the code, allowing improper permissions.
string.HasPrefix("com.microsoft/*", "com.micro") = true
The baseline implementation works because it enforces a .
or /
delimiter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Add delimiter checking after domain prefix (must be . or /)
- Enforce single-slash rule for server names per fix: validate server names to allow only single slash (#471) #476
…tprotocol#481) Implements optional n=<pattern> parameter in DNS TXT records to allow fine-grained permission scoping for server names. This enables DNS controllers to delegate limited namespaces to different teams while maintaining security isolation. Key changes: - Added DNSAuthRecord struct to support name patterns alongside public keys - Collect all valid DNS records instead of stopping at first match - Validate name patterns must start with reverse domain to prevent cross-domain privilege escalation - Add delimiter checking after domain prefix (must be . or /) - Enforce single-slash rule for server names per PR modelcontextprotocol#476 - Added comprehensive tests for all scoping scenarios Example usage: v=MCPv1; k=ed25519; p=<key>; n=com.example/team-foo-* This change addresses Microsoft's requirement for granular permission control within path portions while preserving existing behavior for records without the n= parameter. Fixes modelcontextprotocol#481
7f5dfe5
to
8efede8
Compare
Summary
This PR implements optional server name scoping in DNS TXT records for authentication, allowing fine-grained permission control through an optional
n=<pattern>
parameter.Problem
Currently, DNS TXT records provide wildcard access to ALL subdomains and path portions, making it impossible for DNS controllers to delegate limited namespaces to different teams. This was particularly problematic for enterprise scenarios like Microsoft's, where they need to scope permissions to specific prefixes (e.g.,
com.microsoft/team-foo-*
).Solution
Added support for an optional
n=<pattern>
parameter in DNS TXT records that allows specifying exact permission patterns. When not specified, the system maintains backward compatibility by defaulting to wildcard permissions.Example usage:
Changes
DNSAuthRecord
struct: New data structure to hold both public key and optional name patternn=<pattern>
parameterbuildPermissions()
now uses specific patterns when providedTesting
TestDNSAuthHandler_NamePatternScoping
with 8 test casesn=
is not specifiedBackward Compatibility
This change is fully backward compatible:
n=
parameter continue to work exactly as beforen=
is not specifiedFixes #481