[management] remove regex check from validateDomain()#4000
[management] remove regex check from validateDomain()#4000zvpunry wants to merge 2 commits intonetbirdio:mainfrom
Conversation
|
|
|
The offending tests are for the following dns records, which are expected to fail: But according to https://www.rfc-editor.org/rfc/rfc2181#section-11 these are valid DNS names and shouldn't result in an error. |
zvpunry
force-pushed
the
zvpunry/validateDomain
branch
2 times, most recently
from
ed852c7 to
e0d2f86
Compare
zvpunry
force-pushed
the
zvpunry/validateDomain
branch
2 times, most recently
from
401a45c to
ab14c11
Compare
|
zvpunry
force-pushed
the
zvpunry/validateDomain
branch
from
ab14c11 to
9d51032
Compare
zvpunry
force-pushed
the
zvpunry/validateDomain
branch
from
9d51032 to
b412bb0
Compare
zvpunry
force-pushed
the
zvpunry/validateDomain
branch
from
b412bb0 to
3d1345c
Compare
|
zvpunry
force-pushed
the
zvpunry/validateDomain
branch
from
3d1345c to
aa14700
Compare
📝 WalkthroughWalkthroughDomain validation in the nameserver module was simplified: regex-based checks and matcher were removed and validateDomain() now returns invalid only when dns.IsDomainName(domain) is false. Tests were trimmed and a domain-length overflow test was added. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
management/server/nameserver.go(0 hunks)management/server/nameserver_test.go(1 hunks)
💤 Files with no reviewable changes (1)
- management/server/nameserver.go
zvpunry
force-pushed
the
zvpunry/validateDomain
branch
from
aa14700 to
e17ade9
Compare
zvpunry
force-pushed
the
zvpunry/validateDomain
branch
2 times, most recently
from
b9b5d96 to
fa6492b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
management/cmd/management.go (1)
186-194: Potential race:mgmtPortmay not be finalized whenapplyEmbeddedIdPConfigis called.
loadMgmtConfigis called fromPreRunE, andmgmtPortis conditionally updated later in the samePreRunE(lines 72-79) based on TLS configuration. However,applyEmbeddedIdPConfigis called at line 146 withinloadMgmtConfig, which happens before the port adjustment logic.This means
LocalAddresswill be set using the initialmgmtPortvalue (from flags or default), not the adjusted value (80 or 443 based on TLS).Suggested fix
Consider moving the
LocalAddressassignment to after the port is finalized, or document that users must explicitly set--portwhen using embedded IdP with TLS:- // Set LocalAddress for embedded IdP if enabled, used for internal JWT validation - cfg.EmbeddedIdP.LocalAddress = fmt.Sprintf("localhost:%d", mgmtPort) + // Note: LocalAddress uses the port from --port flag or default. + // When TLS is enabled and port is auto-adjusted later, this may differ. + // For internal JWT validation, localhost access works regardless. + cfg.EmbeddedIdP.LocalAddress = fmt.Sprintf("localhost:%d", mgmtPort)Alternatively, set
LocalAddressinPreRunEafter port finalization.
🤖 Fix all issues with AI agents
In `@management/server/idp/embedded.go`:
- Line 23: The embedded IDP's defaultScopes constant currently omits
"offline_access", preventing refresh tokens; update the defaultScopes (in
management/server/idp/embedded.go) to include "offline_access" (e.g., "openid
profile email offline_access") so the embedded Dex provider can issue refresh
tokens, and also reconcile or document the scopes mismatch for "groups" vs
AUTH_SUPPORTED_SCOPES (either add "groups" to defaultScopes or add a comment/doc
explaining why embedded IDP doesn't expose it). Ensure changes reference the
defaultScopes symbol and update any related docs or comments to reflect the
refresh-token requirement.
🧹 Nitpick comments (1)
management/cmd/management.go (1)
214-221: Consider using constants for hardcoded client IDs and claims.The values
"netbird-dashboard","netbird-cli", and"sub"are defined as constants inmanagement/server/idp/embedded.go(staticClientDashboard,staticClientCLI,defaultUserIDClaim). Using these constants here would improve maintainability and ensure consistency.Suggested refactor
Import the constants from the idp package and use them:
+import "github.com/netbirdio/netbird/management/server/idp" + // Set HttpConfig values from EmbeddedIdP cfg.HttpConfig.AuthIssuer = issuer -cfg.HttpConfig.AuthAudience = "netbird-dashboard" -cfg.HttpConfig.CLIAuthAudience = "netbird-cli" -cfg.HttpConfig.AuthUserIDClaim = "sub" +cfg.HttpConfig.AuthAudience = idp.StaticClientDashboard // Would need to export +cfg.HttpConfig.CLIAuthAudience = idp.StaticClientCLI // Would need to export +cfg.HttpConfig.AuthUserIDClaim = idp.DefaultUserIDClaim // Would need to exportNote: This would require exporting those constants from the idp package.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
idp/dex/provider.goinfrastructure_files/getting-started.shmanagement/cmd/management.gomanagement/server/idp/embedded.gomanagement/server/nameserver.gomanagement/server/nameserver_test.go
💤 Files with no reviewable changes (1)
- management/server/nameserver.go
🚧 Files skipped from review as they are similar to previous changes (1)
- management/server/nameserver_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
management/cmd/management.go (2)
idp/dex/provider.go (1)
Config(30-38)management/internals/server/config/config.go (2)
Config(37-64)HttpServerConfig(97-120)
🔇 Additional comments (3)
infrastructure_files/getting-started.sh (1)
273-273: LGTM - Groups scope added to dashboard environment.The addition of
groupsscope aligns with the OIDC connector changes inidp/dex/provider.gowhereinsecureEnableGroups: trueis now set. This enables group claim propagation from identity providers to NetBird.Note: There's a slight inconsistency with
management/server/idp/embedded.gowheredefaultScopesis"openid profile email"(withoutgroups). The dashboard advertises support for groups, but the embedded IdP defaults don't include it. This should work since the dashboard is the client requesting scopes, but consider aligning the constants for clarity.idp/dex/provider.go (1)
793-813: LGTM - Groups support enabled for OIDC connectors.The changes correctly enable group claims:
insecureEnableGroups: trueallows Dex to pass through group claims from upstream IdPs- Okta and PocketID get explicit
groupsscope since these providers require it to return group informationThe "insecure" prefix in
insecureEnableGroupsis Dex's naming convention (not a security concern) - it simply means groups are enabled without requiring explicit per-connector configuration in Dex's main config.management/cmd/management.go (1)
229-236: LGTM - Correctly skips external OIDC fetch when embedded IdP is enabled.The dual guard pattern is appropriate:
- Early return for empty endpoint (no config to fetch)
- Skip fetch when embedded IdP is enabled (config is provided locally)
This avoids unnecessary network calls and potential startup delays when using the embedded IdP.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
zvpunry
force-pushed
the
zvpunry/validateDomain
branch
3 times, most recently
from
1ed4342 to
c755552
Compare
The regex blocked labels that are allowed by rfc1035 like "x", "x1", "x--x" and possibly some more. The regex check was also redundant because it is directly followed by dns.IsDomainName(domain)
After removing too strict checks from validateDomain(), some tests failed because they expected errors for otherwise valid domain names. This removes the offending tests and adds a test for the maximum length of a domain name.
zvpunry
force-pushed
the
zvpunry/validateDomain
branch
from
c755552 to
b2896c6
Compare
|
|
fixed by #5211 |
2 participants
The regex blocked labels that are allowed by rfc1035 like "x", "x1", "x--x" and possibly some more.
The regex check was also redundant because it is directly followed by dns.IsDomainName(domain)
Describe your changes
I only removed the regex check from validateDomain().
This change still requires an update to management/server/nameserver_test.go because it tests for the too strict domain validation.
Issue ticket number and link
#3996
Stack
Checklist
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.