[codex] Add access control flags to netbird expose#6675
Draft
constantins2001 wants to merge 2 commits into
Draft
[codex] Add access control flags to netbird expose#6675constantins2001 wants to merge 2 commits into
constantins2001 wants to merge 2 commits into
Conversation
|
|
Contributor
📝 WalkthroughWalkthroughThis PR adds allow/block IP, CIDR, and country access restrictions to ChangesExpose Access Restrictions Feature
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ExposeCmd
participant buildExposeAccessRestrictions
participant DaemonRequest
participant ManagementGRPC
participant ReverseProxyService
User->>ExposeCmd: netbird expose --allow-ip/--block-cidr/--allow-country
ExposeCmd->>buildExposeAccessRestrictions: parse and normalize flags
buildExposeAccessRestrictions-->>ExposeCmd: AccessRestrictions or nil
ExposeCmd->>DaemonRequest: proto.ExposeServiceRequest.AccessRestrictions
DaemonRequest->>ManagementGRPC: expose request conversion
ManagementGRPC->>ReverseProxyService: Restrictions
ReverseProxyService->>ReverseProxyService: Validate() and ToService()
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
management/internals/shared/grpc/expose_service.go (1)
55-66: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winMap
crowdsec_modethroughexposeAccessRestrictionsFromProto
This helper drops the sharedAccessRestrictions.crowdsec_modefield, so callers can set CrowdSec mode inExposeService.access_restrictionsbut it will be silently ignored. Carry it through torpservice.AccessRestrictions(or reject it explicitly if this RPC shouldn’t accept it).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@management/internals/shared/grpc/expose_service.go` around lines 55 - 66, The ExposeServiceRequest construction in expose_service.go is dropping AccessRestrictions.crowdsec_mode because exposeAccessRestrictionsFromProto does not carry it through. Update exposeAccessRestrictionsFromProto and the rpservice.AccessRestrictions mapping used by CreateServiceFromPeer so crowdsec_mode is preserved end-to-end, or explicitly reject it in the ExposeService RPC if that field is not supported.
🧹 Nitpick comments (1)
client/cmd/expose.go (1)
215-225: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCountry code check only verifies length, not that it's alphabetic.
normalizeExposeCountryCodesaccepts any 2-character string (e.g."12","$$") since it only checkslen(code) != 2. Invalid codes will only be caught later by management validation, after a daemon round-trip, giving a less immediate error than validating locally.♻️ Suggested tightening
func normalizeExposeCountryCodes(flag string, codes []string) ([]string, error) { normalized := make([]string, 0, len(codes)) for _, raw := range codes { code := strings.ToUpper(strings.TrimSpace(raw)) - if len(code) != 2 { + if len(code) != 2 || code[0] < 'A' || code[0] > 'Z' || code[1] < 'A' || code[1] > 'Z' { return nil, fmt.Errorf("--%s %q must be a 2-letter ISO 3166-1 alpha-2 country code", flag, raw) } normalized = append(normalized, code) } return normalized, nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/cmd/expose.go` around lines 215 - 225, The validation in normalizeExposeCountryCodes only checks that each country code has length 2, so non-letter values can pass through. Tighten the local check in this function by also verifying the normalized code is alphabetic before appending it, and return the same kind of fmt.Errorf for invalid inputs. Use the existing normalizeExposeCountryCodes helper and its country-code parsing logic to keep the fix localized.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@management/internals/shared/grpc/expose_service.go`:
- Around line 55-66: The ExposeServiceRequest construction in expose_service.go
is dropping AccessRestrictions.crowdsec_mode because
exposeAccessRestrictionsFromProto does not carry it through. Update
exposeAccessRestrictionsFromProto and the rpservice.AccessRestrictions mapping
used by CreateServiceFromPeer so crowdsec_mode is preserved end-to-end, or
explicitly reject it in the ExposeService RPC if that field is not supported.
---
Nitpick comments:
In `@client/cmd/expose.go`:
- Around line 215-225: The validation in normalizeExposeCountryCodes only checks
that each country code has length 2, so non-letter values can pass through.
Tighten the local check in this function by also verifying the normalized code
is alphabetic before appending it, and return the same kind of fmt.Errorf for
invalid inputs. Use the existing normalizeExposeCountryCodes helper and its
country-code parsing logic to keep the fix localized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6a772375-2350-4f1b-813c-b66ad3e060a9
⛔ Files ignored due to path filters (2)
client/proto/daemon.pb.gois excluded by!**/*.pb.goshared/management/proto/management.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (12)
client/cmd/expose.goclient/cmd/expose_test.goclient/internal/expose/manager.goclient/internal/expose/manager_test.goclient/internal/expose/request.goclient/proto/daemon.protomanagement/internals/modules/reverseproxy/service/service.gomanagement/internals/modules/reverseproxy/service/service_test.gomanagement/internals/shared/grpc/expose_service.goshared/management/client/grpc.goshared/management/client/grpc_expose_test.goshared/management/proto/management.proto
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Describe your changes
Adds CLI parity for reverse-proxy Access Control Rules when creating peer-initiated expose services with
netbird expose.The new flags are:
--allow-ip--block-ip--allow-cidr--block-cidr--allow-country--block-countrySingle IP flags are normalized to
/32or/128CIDRs before being sent to management, matching the existing reverse-proxy access restriction model.Implementation details:
AccessRestrictionsbackend model and validation.crowdsec_modewhen peer-created expose services receive the sharedAccessRestrictionsproto.Issue ticket number and link
Closes #6673
Discussion-first validation: #6676
Stack
Single-PR change; no stacked dependencies.
Checklist
This PR is intentionally kept as draft while the discussion-first validation happens in #6676.
Documentation
Select exactly one:
The CLI help output is updated by the Cobra flags in this PR. External docs can be added in
netbirdio/docsafter maintainers confirm the CLI/API shape.Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
https://github.com/netbirdio/docs/pull/__
Validation
GOCACHE=/private/tmp/netbird-go-build-cache GOMODCACHE=/private/tmp/netbird-go-mod-cache go test ./client/cmd ./management/internals/shared/grpcGOCACHE=/private/tmp/netbird-go-build-cache GOMODCACHE=/private/tmp/netbird-go-mod-cache go test ./client/cmd ./client/internal/expose ./shared/management/client ./management/internals/modules/reverseproxy/service ./management/internals/shared/grpc ./management/internals/modules/reverseproxy/service/managergit diff --check