diff --git a/client/cmd/debug.go b/client/cmd/debug.go index b4adee826..385bd95f5 100644 --- a/client/cmd/debug.go +++ b/client/cmd/debug.go @@ -235,13 +235,6 @@ func runForDuration(cmd *cobra.Command, args []string) error { return fmt.Errorf("failed to bundle debug: %v", status.Convert(err).Message()) } - // Disable network map persistence after creating the debug bundle - if _, err := client.SetNetworkMapPersistence(cmd.Context(), &proto.SetNetworkMapPersistenceRequest{ - Enabled: false, - }); err != nil { - return fmt.Errorf("failed to disable network map persistence: %v", status.Convert(err).Message()) - } - if stateWasDown { if _, err := client.Down(cmd.Context(), &proto.DownRequest{}); err != nil { return fmt.Errorf("failed to down: %v", status.Convert(err).Message()) diff --git a/client/cmd/root.go b/client/cmd/root.go index b4f067078..b57bee230 100644 --- a/client/cmd/root.go +++ b/client/cmd/root.go @@ -42,7 +42,6 @@ const ( blockLANAccessFlag = "block-lan-access" uploadBundle = "upload-bundle" uploadBundleURL = "upload-bundle-url" - defaultBundleURL = "https://upload.debug.netbird.io" + types.GetURLPath ) var ( @@ -188,7 +187,7 @@ func init() { debugCmd.PersistentFlags().BoolVarP(&debugSystemInfoFlag, systemInfoFlag, "S", true, "Adds system information to the debug bundle") debugCmd.PersistentFlags().BoolVarP(&debugUploadBundle, uploadBundle, "U", false, fmt.Sprintf("Uploads the debug bundle to a server from URL defined by %s", uploadBundleURL)) - debugCmd.PersistentFlags().StringVar(&debugUploadBundleURL, uploadBundleURL, defaultBundleURL, "Service URL to get an URL to upload the debug bundle") + debugCmd.PersistentFlags().StringVar(&debugUploadBundleURL, uploadBundleURL, types.DefaultBundleURL, "Service URL to get an URL to upload the debug bundle") } // SetupCloseHandler handles SIGTERM signal and exits with success diff --git a/client/internal/dns/handler_chain_test.go b/client/internal/dns/handler_chain_test.go index 4c910a95f..5f03e0758 100644 --- a/client/internal/dns/handler_chain_test.go +++ b/client/internal/dns/handler_chain_test.go @@ -1,7 +1,6 @@ package dns_test import ( - "net" "testing" "github.com/miekg/dns" @@ -9,6 +8,7 @@ import ( "github.com/stretchr/testify/mock" nbdns "github.com/netbirdio/netbird/client/internal/dns" + "github.com/netbirdio/netbird/client/internal/dns/test" ) // TestHandlerChain_ServeDNS_Priorities tests that handlers are executed in priority order @@ -30,7 +30,7 @@ func TestHandlerChain_ServeDNS_Priorities(t *testing.T) { r.SetQuestion("example.com.", dns.TypeA) // Create test writer - w := &nbdns.ResponseWriterChain{ResponseWriter: &mockResponseWriter{}} + w := &nbdns.ResponseWriterChain{ResponseWriter: &test.MockResponseWriter{}} // Setup expectations - only highest priority handler should be called dnsRouteHandler.On("ServeDNS", mock.Anything, r).Once() @@ -142,7 +142,7 @@ func TestHandlerChain_ServeDNS_DomainMatching(t *testing.T) { r := new(dns.Msg) r.SetQuestion(tt.queryDomain, dns.TypeA) - w := &nbdns.ResponseWriterChain{ResponseWriter: &mockResponseWriter{}} + w := &nbdns.ResponseWriterChain{ResponseWriter: &test.MockResponseWriter{}} chain.ServeDNS(w, r) @@ -259,7 +259,7 @@ func TestHandlerChain_ServeDNS_OverlappingDomains(t *testing.T) { // Create and execute request r := new(dns.Msg) r.SetQuestion(tt.queryDomain, dns.TypeA) - w := &nbdns.ResponseWriterChain{ResponseWriter: &mockResponseWriter{}} + w := &nbdns.ResponseWriterChain{ResponseWriter: &test.MockResponseWriter{}} chain.ServeDNS(w, r) // Verify expectations @@ -316,7 +316,7 @@ func TestHandlerChain_ServeDNS_ChainContinuation(t *testing.T) { }).Once() // Execute - w := &nbdns.ResponseWriterChain{ResponseWriter: &mockResponseWriter{}} + w := &nbdns.ResponseWriterChain{ResponseWriter: &test.MockResponseWriter{}} chain.ServeDNS(w, r) // Verify all handlers were called in order @@ -325,20 +325,6 @@ func TestHandlerChain_ServeDNS_ChainContinuation(t *testing.T) { handler3.AssertExpectations(t) } -// mockResponseWriter implements dns.ResponseWriter for testing -type mockResponseWriter struct { - mock.Mock -} - -func (m *mockResponseWriter) LocalAddr() net.Addr { return nil } -func (m *mockResponseWriter) RemoteAddr() net.Addr { return nil } -func (m *mockResponseWriter) WriteMsg(*dns.Msg) error { return nil } -func (m *mockResponseWriter) Write([]byte) (int, error) { return 0, nil } -func (m *mockResponseWriter) Close() error { return nil } -func (m *mockResponseWriter) TsigStatus() error { return nil } -func (m *mockResponseWriter) TsigTimersOnly(bool) {} -func (m *mockResponseWriter) Hijack() {} - func TestHandlerChain_PriorityDeregistration(t *testing.T) { tests := []struct { name string @@ -425,7 +411,7 @@ func TestHandlerChain_PriorityDeregistration(t *testing.T) { // Create test request r := new(dns.Msg) r.SetQuestion(tt.query, dns.TypeA) - w := &nbdns.ResponseWriterChain{ResponseWriter: &mockResponseWriter{}} + w := &nbdns.ResponseWriterChain{ResponseWriter: &test.MockResponseWriter{}} // Setup expectations for priority, handler := range handlers { @@ -471,7 +457,7 @@ func TestHandlerChain_MultiPriorityHandling(t *testing.T) { chain.AddHandler(testDomain, matchHandler, nbdns.PriorityMatchDomain) // Test 1: Initial state - w1 := &nbdns.ResponseWriterChain{ResponseWriter: &mockResponseWriter{}} + w1 := &nbdns.ResponseWriterChain{ResponseWriter: &test.MockResponseWriter{}} // Highest priority handler (routeHandler) should be called routeHandler.On("ServeDNS", mock.Anything, r).Return().Once() matchHandler.On("ServeDNS", mock.Anything, r).Maybe() // Ensure others are not expected yet @@ -490,7 +476,7 @@ func TestHandlerChain_MultiPriorityHandling(t *testing.T) { // Test 2: Remove highest priority handler chain.RemoveHandler(testDomain, nbdns.PriorityDNSRoute) - w2 := &nbdns.ResponseWriterChain{ResponseWriter: &mockResponseWriter{}} + w2 := &nbdns.ResponseWriterChain{ResponseWriter: &test.MockResponseWriter{}} // Now middle priority handler (matchHandler) should be called matchHandler.On("ServeDNS", mock.Anything, r).Return().Once() defaultHandler.On("ServeDNS", mock.Anything, r).Maybe() // Ensure default is not expected yet @@ -506,7 +492,7 @@ func TestHandlerChain_MultiPriorityHandling(t *testing.T) { // Test 3: Remove middle priority handler chain.RemoveHandler(testDomain, nbdns.PriorityMatchDomain) - w3 := &nbdns.ResponseWriterChain{ResponseWriter: &mockResponseWriter{}} + w3 := &nbdns.ResponseWriterChain{ResponseWriter: &test.MockResponseWriter{}} // Now lowest priority handler (defaultHandler) should be called defaultHandler.On("ServeDNS", mock.Anything, r).Return().Once() @@ -519,7 +505,7 @@ func TestHandlerChain_MultiPriorityHandling(t *testing.T) { // Test 4: Remove last handler chain.RemoveHandler(testDomain, nbdns.PriorityDefault) - w4 := &nbdns.ResponseWriterChain{ResponseWriter: &mockResponseWriter{}} + w4 := &nbdns.ResponseWriterChain{ResponseWriter: &test.MockResponseWriter{}} chain.ServeDNS(w4, r) // Call ServeDNS on the now empty chain for this domain for _, m := range mocks { @@ -675,7 +661,7 @@ func TestHandlerChain_CaseSensitivity(t *testing.T) { // Execute request r := new(dns.Msg) r.SetQuestion(tt.query, dns.TypeA) - chain.ServeDNS(&mockResponseWriter{}, r) + chain.ServeDNS(&test.MockResponseWriter{}, r) // Verify each handler was called exactly as expected for _, h := range tt.addHandlers { @@ -819,7 +805,7 @@ func TestHandlerChain_DomainSpecificityOrdering(t *testing.T) { r := new(dns.Msg) r.SetQuestion(tt.query, dns.TypeA) - w := &nbdns.ResponseWriterChain{ResponseWriter: &mockResponseWriter{}} + w := &nbdns.ResponseWriterChain{ResponseWriter: &test.MockResponseWriter{}} // Setup handler expectations for pattern, handler := range handlers { @@ -969,7 +955,7 @@ func TestHandlerChain_AddRemoveRoundtrip(t *testing.T) { handler := &nbdns.MockHandler{} r := new(dns.Msg) r.SetQuestion(tt.queryPattern, dns.TypeA) - w := &nbdns.ResponseWriterChain{ResponseWriter: &mockResponseWriter{}} + w := &nbdns.ResponseWriterChain{ResponseWriter: &test.MockResponseWriter{}} // First verify no handler is called before adding any chain.ServeDNS(w, r) diff --git a/client/internal/dns/local.go b/client/internal/dns/local.go deleted file mode 100644 index 76e18e3ce..000000000 --- a/client/internal/dns/local.go +++ /dev/null @@ -1,130 +0,0 @@ -package dns - -import ( - "fmt" - "strings" - "sync" - - "github.com/miekg/dns" - log "github.com/sirupsen/logrus" - - nbdns "github.com/netbirdio/netbird/dns" -) - -type registrationMap map[string]struct{} - -type localResolver struct { - registeredMap registrationMap - records sync.Map // key: string (domain_class_type), value: []dns.RR -} - -func (d *localResolver) MatchSubdomains() bool { - return true -} - -func (d *localResolver) stop() { -} - -// String returns a string representation of the local resolver -func (d *localResolver) String() string { - return fmt.Sprintf("local resolver [%d records]", len(d.registeredMap)) -} - -// ID returns the unique handler ID -func (d *localResolver) id() handlerID { - return "local-resolver" -} - -// ServeDNS handles a DNS request -func (d *localResolver) ServeDNS(w dns.ResponseWriter, r *dns.Msg) { - if len(r.Question) > 0 { - log.Tracef("received local question: domain=%s type=%v class=%v", r.Question[0].Name, r.Question[0].Qtype, r.Question[0].Qclass) - } - - replyMessage := &dns.Msg{} - replyMessage.SetReply(r) - replyMessage.RecursionAvailable = true - - // lookup all records matching the question - records := d.lookupRecords(r) - if len(records) > 0 { - replyMessage.Rcode = dns.RcodeSuccess - replyMessage.Answer = append(replyMessage.Answer, records...) - } else { - replyMessage.Rcode = dns.RcodeNameError - } - - err := w.WriteMsg(replyMessage) - if err != nil { - log.Debugf("got an error while writing the local resolver response, error: %v", err) - } -} - -// lookupRecords fetches *all* DNS records matching the first question in r. -func (d *localResolver) lookupRecords(r *dns.Msg) []dns.RR { - if len(r.Question) == 0 { - return nil - } - question := r.Question[0] - question.Name = strings.ToLower(question.Name) - key := buildRecordKey(question.Name, question.Qclass, question.Qtype) - - value, found := d.records.Load(key) - if !found { - // alternatively check if we have a cname - if question.Qtype != dns.TypeCNAME { - r.Question[0].Qtype = dns.TypeCNAME - return d.lookupRecords(r) - } - - return nil - } - - records, ok := value.([]dns.RR) - if !ok { - log.Errorf("failed to cast records to []dns.RR, records: %v", value) - return nil - } - - // if there's more than one record, rotate them (round-robin) - if len(records) > 1 { - first := records[0] - records = append(records[1:], first) - d.records.Store(key, records) - } - - return records -} - -// registerRecord stores a new record by appending it to any existing list -func (d *localResolver) registerRecord(record nbdns.SimpleRecord) (string, error) { - rr, err := dns.NewRR(record.String()) - if err != nil { - return "", fmt.Errorf("register record: %w", err) - } - - rr.Header().Rdlength = record.Len() - header := rr.Header() - key := buildRecordKey(header.Name, header.Class, header.Rrtype) - - // load any existing slice of records, then append - existing, _ := d.records.LoadOrStore(key, []dns.RR{}) - records := existing.([]dns.RR) - records = append(records, rr) - - // store updated slice - d.records.Store(key, records) - return key, nil -} - -// deleteRecord removes *all* records under the recordKey. -func (d *localResolver) deleteRecord(recordKey string) { - d.records.Delete(dns.Fqdn(recordKey)) -} - -// buildRecordKey consistently generates a key: name_class_type -func buildRecordKey(name string, class, qType uint16) string { - return fmt.Sprintf("%s_%d_%d", dns.Fqdn(name), class, qType) -} - -func (d *localResolver) probeAvailability() {} diff --git a/client/internal/dns/local/local.go b/client/internal/dns/local/local.go new file mode 100644 index 000000000..de3d8514b --- /dev/null +++ b/client/internal/dns/local/local.go @@ -0,0 +1,149 @@ +package local + +import ( + "fmt" + "slices" + "strings" + "sync" + + "github.com/miekg/dns" + log "github.com/sirupsen/logrus" + "golang.org/x/exp/maps" + + "github.com/netbirdio/netbird/client/internal/dns/types" + nbdns "github.com/netbirdio/netbird/dns" +) + +type Resolver struct { + mu sync.RWMutex + records map[dns.Question][]dns.RR +} + +func NewResolver() *Resolver { + return &Resolver{ + records: make(map[dns.Question][]dns.RR), + } +} + +func (d *Resolver) MatchSubdomains() bool { + return true +} + +// String returns a string representation of the local resolver +func (d *Resolver) String() string { + return fmt.Sprintf("local resolver [%d records]", len(d.records)) +} + +func (d *Resolver) Stop() {} + +// ID returns the unique handler ID +func (d *Resolver) ID() types.HandlerID { + return "local-resolver" +} + +func (d *Resolver) ProbeAvailability() {} + +// ServeDNS handles a DNS request +func (d *Resolver) ServeDNS(w dns.ResponseWriter, r *dns.Msg) { + if len(r.Question) == 0 { + log.Debugf("received local resolver request with no question") + return + } + question := r.Question[0] + question.Name = strings.ToLower(dns.Fqdn(question.Name)) + + log.Tracef("received local question: domain=%s type=%v class=%v", r.Question[0].Name, question.Qtype, question.Qclass) + + replyMessage := &dns.Msg{} + replyMessage.SetReply(r) + replyMessage.RecursionAvailable = true + + // lookup all records matching the question + records := d.lookupRecords(question) + if len(records) > 0 { + replyMessage.Rcode = dns.RcodeSuccess + replyMessage.Answer = append(replyMessage.Answer, records...) + } else { + // TODO: return success if we have a different record type for the same name, relevant for search domains + replyMessage.Rcode = dns.RcodeNameError + } + + if err := w.WriteMsg(replyMessage); err != nil { + log.Warnf("failed to write the local resolver response: %v", err) + } +} + +// lookupRecords fetches *all* DNS records matching the first question in r. +func (d *Resolver) lookupRecords(question dns.Question) []dns.RR { + d.mu.RLock() + records, found := d.records[question] + + if !found { + d.mu.RUnlock() + // alternatively check if we have a cname + if question.Qtype != dns.TypeCNAME { + question.Qtype = dns.TypeCNAME + return d.lookupRecords(question) + } + return nil + } + + recordsCopy := slices.Clone(records) + d.mu.RUnlock() + + // if there's more than one record, rotate them (round-robin) + if len(recordsCopy) > 1 { + d.mu.Lock() + records = d.records[question] + if len(records) > 1 { + first := records[0] + records = append(records[1:], first) + d.records[question] = records + } + d.mu.Unlock() + } + + return recordsCopy +} + +func (d *Resolver) Update(update []nbdns.SimpleRecord) { + d.mu.Lock() + defer d.mu.Unlock() + + maps.Clear(d.records) + + for _, rec := range update { + if err := d.registerRecord(rec); err != nil { + log.Warnf("failed to register the record (%s): %v", rec, err) + continue + } + } +} + +// RegisterRecord stores a new record by appending it to any existing list +func (d *Resolver) RegisterRecord(record nbdns.SimpleRecord) error { + d.mu.Lock() + defer d.mu.Unlock() + + return d.registerRecord(record) +} + +// registerRecord performs the registration with the lock already held +func (d *Resolver) registerRecord(record nbdns.SimpleRecord) error { + rr, err := dns.NewRR(record.String()) + if err != nil { + return fmt.Errorf("register record: %w", err) + } + + rr.Header().Rdlength = record.Len() + header := rr.Header() + q := dns.Question{ + Name: strings.ToLower(dns.Fqdn(header.Name)), + Qtype: header.Rrtype, + Qclass: header.Class, + } + + d.records[q] = append(d.records[q], rr) + + return nil +} diff --git a/client/internal/dns/local/local_test.go b/client/internal/dns/local/local_test.go new file mode 100644 index 000000000..1d38191e7 --- /dev/null +++ b/client/internal/dns/local/local_test.go @@ -0,0 +1,472 @@ +package local + +import ( + "strings" + "testing" + + "github.com/miekg/dns" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/netbirdio/netbird/client/internal/dns/test" + nbdns "github.com/netbirdio/netbird/dns" +) + +func TestLocalResolver_ServeDNS(t *testing.T) { + recordA := nbdns.SimpleRecord{ + Name: "peera.netbird.cloud.", + Type: 1, + Class: nbdns.DefaultClass, + TTL: 300, + RData: "1.2.3.4", + } + + recordCNAME := nbdns.SimpleRecord{ + Name: "peerb.netbird.cloud.", + Type: 5, + Class: nbdns.DefaultClass, + TTL: 300, + RData: "www.netbird.io", + } + + testCases := []struct { + name string + inputRecord nbdns.SimpleRecord + inputMSG *dns.Msg + responseShouldBeNil bool + }{ + { + name: "Should Resolve A Record", + inputRecord: recordA, + inputMSG: new(dns.Msg).SetQuestion(recordA.Name, dns.TypeA), + }, + { + name: "Should Resolve CNAME Record", + inputRecord: recordCNAME, + inputMSG: new(dns.Msg).SetQuestion(recordCNAME.Name, dns.TypeCNAME), + }, + { + name: "Should Not Write When Not Found A Record", + inputRecord: recordA, + inputMSG: new(dns.Msg).SetQuestion("not.found.com", dns.TypeA), + responseShouldBeNil: true, + }, + } + + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + resolver := NewResolver() + _ = resolver.RegisterRecord(testCase.inputRecord) + var responseMSG *dns.Msg + responseWriter := &test.MockResponseWriter{ + WriteMsgFunc: func(m *dns.Msg) error { + responseMSG = m + return nil + }, + } + + resolver.ServeDNS(responseWriter, testCase.inputMSG) + + if responseMSG == nil || len(responseMSG.Answer) == 0 { + if testCase.responseShouldBeNil { + return + } + t.Fatalf("should write a response message") + } + + answerString := responseMSG.Answer[0].String() + if !strings.Contains(answerString, testCase.inputRecord.Name) { + t.Fatalf("answer doesn't contain the same domain name: \nWant: %s\nGot:%s", testCase.name, answerString) + } + if !strings.Contains(answerString, dns.Type(testCase.inputRecord.Type).String()) { + t.Fatalf("answer doesn't contain the correct type: \nWant: %s\nGot:%s", dns.Type(testCase.inputRecord.Type).String(), answerString) + } + if !strings.Contains(answerString, testCase.inputRecord.RData) { + t.Fatalf("answer doesn't contain the same address: \nWant: %s\nGot:%s", testCase.inputRecord.RData, answerString) + } + }) + } +} + +// TestLocalResolver_Update_StaleRecord verifies that updating +// a record correctly replaces the old one, preventing stale entries. +func TestLocalResolver_Update_StaleRecord(t *testing.T) { + recordName := "host.example.com." + recordType := dns.TypeA + recordClass := dns.ClassINET + + record1 := nbdns.SimpleRecord{ + Name: recordName, Type: int(recordType), Class: nbdns.DefaultClass, TTL: 300, RData: "1.1.1.1", + } + record2 := nbdns.SimpleRecord{ + Name: recordName, Type: int(recordType), Class: nbdns.DefaultClass, TTL: 300, RData: "2.2.2.2", + } + + recordKey := dns.Question{Name: recordName, Qtype: uint16(recordClass), Qclass: recordType} + + resolver := NewResolver() + + update1 := []nbdns.SimpleRecord{record1} + update2 := []nbdns.SimpleRecord{record2} + + // Apply first update + resolver.Update(update1) + + // Verify first update + resolver.mu.RLock() + rrSlice1, found1 := resolver.records[recordKey] + resolver.mu.RUnlock() + + require.True(t, found1, "Record key %s not found after first update", recordKey) + require.Len(t, rrSlice1, 1, "Should have exactly 1 record after first update") + assert.Contains(t, rrSlice1[0].String(), record1.RData, "Record after first update should be %s", record1.RData) + + // Apply second update + resolver.Update(update2) + + // Verify second update + resolver.mu.RLock() + rrSlice2, found2 := resolver.records[recordKey] + resolver.mu.RUnlock() + + require.True(t, found2, "Record key %s not found after second update", recordKey) + require.Len(t, rrSlice2, 1, "Should have exactly 1 record after update overwriting the key") + assert.Contains(t, rrSlice2[0].String(), record2.RData, "The single record should be the updated one (%s)", record2.RData) + assert.NotContains(t, rrSlice2[0].String(), record1.RData, "The stale record (%s) should not be present", record1.RData) +} + +// TestLocalResolver_MultipleRecords_SameQuestion verifies that multiple records +// with the same question are stored properly +func TestLocalResolver_MultipleRecords_SameQuestion(t *testing.T) { + resolver := NewResolver() + + recordName := "multi.example.com." + recordType := dns.TypeA + + // Create two records with the same name and type but different IPs + record1 := nbdns.SimpleRecord{ + Name: recordName, Type: int(recordType), Class: nbdns.DefaultClass, TTL: 300, RData: "10.0.0.1", + } + record2 := nbdns.SimpleRecord{ + Name: recordName, Type: int(recordType), Class: nbdns.DefaultClass, TTL: 300, RData: "10.0.0.2", + } + + update := []nbdns.SimpleRecord{record1, record2} + + // Apply update with both records + resolver.Update(update) + + // Create question that matches both records + question := dns.Question{ + Name: recordName, + Qtype: recordType, + Qclass: dns.ClassINET, + } + + // Verify both records are stored + resolver.mu.RLock() + records, found := resolver.records[question] + resolver.mu.RUnlock() + + require.True(t, found, "Records for question %v not found", question) + require.Len(t, records, 2, "Should have exactly 2 records for the same question") + + // Verify both record data values are present + recordStrings := []string{records[0].String(), records[1].String()} + assert.Contains(t, recordStrings[0]+recordStrings[1], record1.RData, "First record data should be present") + assert.Contains(t, recordStrings[0]+recordStrings[1], record2.RData, "Second record data should be present") +} + +// TestLocalResolver_RecordRotation verifies that records are rotated in a round-robin fashion +func TestLocalResolver_RecordRotation(t *testing.T) { + resolver := NewResolver() + + recordName := "rotation.example.com." + recordType := dns.TypeA + + // Create three records with the same name and type but different IPs + record1 := nbdns.SimpleRecord{ + Name: recordName, Type: int(recordType), Class: nbdns.DefaultClass, TTL: 300, RData: "192.168.1.1", + } + record2 := nbdns.SimpleRecord{ + Name: recordName, Type: int(recordType), Class: nbdns.DefaultClass, TTL: 300, RData: "192.168.1.2", + } + record3 := nbdns.SimpleRecord{ + Name: recordName, Type: int(recordType), Class: nbdns.DefaultClass, TTL: 300, RData: "192.168.1.3", + } + + update := []nbdns.SimpleRecord{record1, record2, record3} + + // Apply update with all three records + resolver.Update(update) + + msg := new(dns.Msg).SetQuestion(recordName, recordType) + + // First lookup - should return the records in original order + var responses [3]*dns.Msg + + // Perform three lookups to verify rotation + for i := 0; i < 3; i++ { + responseWriter := &test.MockResponseWriter{ + WriteMsgFunc: func(m *dns.Msg) error { + responses[i] = m + return nil + }, + } + + resolver.ServeDNS(responseWriter, msg) + } + + // Verify all three responses contain answers + for i, resp := range responses { + require.NotNil(t, resp, "Response %d should not be nil", i) + require.Len(t, resp.Answer, 3, "Response %d should have 3 answers", i) + } + + // Verify the first record in each response is different due to rotation + firstRecordIPs := []string{ + responses[0].Answer[0].String(), + responses[1].Answer[0].String(), + responses[2].Answer[0].String(), + } + + // Each record should be different (rotated) + assert.NotEqual(t, firstRecordIPs[0], firstRecordIPs[1], "First lookup should differ from second lookup due to rotation") + assert.NotEqual(t, firstRecordIPs[1], firstRecordIPs[2], "Second lookup should differ from third lookup due to rotation") + assert.NotEqual(t, firstRecordIPs[0], firstRecordIPs[2], "First lookup should differ from third lookup due to rotation") + + // After three rotations, we should have cycled through all records + assert.Contains(t, firstRecordIPs[0]+firstRecordIPs[1]+firstRecordIPs[2], record1.RData) + assert.Contains(t, firstRecordIPs[0]+firstRecordIPs[1]+firstRecordIPs[2], record2.RData) + assert.Contains(t, firstRecordIPs[0]+firstRecordIPs[1]+firstRecordIPs[2], record3.RData) +} + +// TestLocalResolver_CaseInsensitiveMatching verifies that DNS record lookups are case-insensitive +func TestLocalResolver_CaseInsensitiveMatching(t *testing.T) { + resolver := NewResolver() + + // Create record with lowercase name + lowerCaseRecord := nbdns.SimpleRecord{ + Name: "lower.example.com.", + Type: int(dns.TypeA), + Class: nbdns.DefaultClass, + TTL: 300, + RData: "10.10.10.10", + } + + // Create record with mixed case name + mixedCaseRecord := nbdns.SimpleRecord{ + Name: "MiXeD.ExAmPlE.CoM.", + Type: int(dns.TypeA), + Class: nbdns.DefaultClass, + TTL: 300, + RData: "20.20.20.20", + } + + // Update resolver with the records + resolver.Update([]nbdns.SimpleRecord{lowerCaseRecord, mixedCaseRecord}) + + testCases := []struct { + name string + queryName string + expectedRData string + shouldResolve bool + }{ + { + name: "Query lowercase with lowercase record", + queryName: "lower.example.com.", + expectedRData: "10.10.10.10", + shouldResolve: true, + }, + { + name: "Query uppercase with lowercase record", + queryName: "LOWER.EXAMPLE.COM.", + expectedRData: "10.10.10.10", + shouldResolve: true, + }, + { + name: "Query mixed case with lowercase record", + queryName: "LoWeR.eXaMpLe.CoM.", + expectedRData: "10.10.10.10", + shouldResolve: true, + }, + { + name: "Query lowercase with mixed case record", + queryName: "mixed.example.com.", + expectedRData: "20.20.20.20", + shouldResolve: true, + }, + { + name: "Query uppercase with mixed case record", + queryName: "MIXED.EXAMPLE.COM.", + expectedRData: "20.20.20.20", + shouldResolve: true, + }, + { + name: "Query with different casing pattern", + queryName: "mIxEd.ExaMpLe.cOm.", + expectedRData: "20.20.20.20", + shouldResolve: true, + }, + { + name: "Query non-existent domain", + queryName: "nonexistent.example.com.", + shouldResolve: false, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + var responseMSG *dns.Msg + + // Create DNS query with the test case name + msg := new(dns.Msg).SetQuestion(tc.queryName, dns.TypeA) + + // Create mock response writer to capture the response + responseWriter := &test.MockResponseWriter{ + WriteMsgFunc: func(m *dns.Msg) error { + responseMSG = m + return nil + }, + } + + // Perform DNS query + resolver.ServeDNS(responseWriter, msg) + + // Check if we expect a successful resolution + if !tc.shouldResolve { + if responseMSG == nil || len(responseMSG.Answer) == 0 { + // Expected no answer, test passes + return + } + t.Fatalf("Expected no resolution for %s, but got answer: %v", tc.queryName, responseMSG.Answer) + } + + // Verify we got a response + require.NotNil(t, responseMSG, "Should have received a response message") + require.Greater(t, len(responseMSG.Answer), 0, "Response should contain at least one answer") + + // Verify the response contains the expected data + answerString := responseMSG.Answer[0].String() + assert.Contains(t, answerString, tc.expectedRData, + "Answer should contain the expected IP address %s, got: %s", + tc.expectedRData, answerString) + }) + } +} + +// TestLocalResolver_CNAMEFallback verifies that the resolver correctly falls back +// to checking for CNAME records when the requested record type isn't found +func TestLocalResolver_CNAMEFallback(t *testing.T) { + resolver := NewResolver() + + // Create a CNAME record (but no A record for this name) + cnameRecord := nbdns.SimpleRecord{ + Name: "alias.example.com.", + Type: int(dns.TypeCNAME), + Class: nbdns.DefaultClass, + TTL: 300, + RData: "target.example.com.", + } + + // Create an A record for the CNAME target + targetRecord := nbdns.SimpleRecord{ + Name: "target.example.com.", + Type: int(dns.TypeA), + Class: nbdns.DefaultClass, + TTL: 300, + RData: "192.168.100.100", + } + + // Update resolver with both records + resolver.Update([]nbdns.SimpleRecord{cnameRecord, targetRecord}) + + testCases := []struct { + name string + queryName string + queryType uint16 + expectedType string + expectedRData string + shouldResolve bool + }{ + { + name: "Directly query CNAME record", + queryName: "alias.example.com.", + queryType: dns.TypeCNAME, + expectedType: "CNAME", + expectedRData: "target.example.com.", + shouldResolve: true, + }, + { + name: "Query A record but get CNAME fallback", + queryName: "alias.example.com.", + queryType: dns.TypeA, + expectedType: "CNAME", + expectedRData: "target.example.com.", + shouldResolve: true, + }, + { + name: "Query AAAA record but get CNAME fallback", + queryName: "alias.example.com.", + queryType: dns.TypeAAAA, + expectedType: "CNAME", + expectedRData: "target.example.com.", + shouldResolve: true, + }, + { + name: "Query direct A record", + queryName: "target.example.com.", + queryType: dns.TypeA, + expectedType: "A", + expectedRData: "192.168.100.100", + shouldResolve: true, + }, + { + name: "Query non-existent name", + queryName: "nonexistent.example.com.", + queryType: dns.TypeA, + shouldResolve: false, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + var responseMSG *dns.Msg + + // Create DNS query with the test case parameters + msg := new(dns.Msg).SetQuestion(tc.queryName, tc.queryType) + + // Create mock response writer to capture the response + responseWriter := &test.MockResponseWriter{ + WriteMsgFunc: func(m *dns.Msg) error { + responseMSG = m + return nil + }, + } + + // Perform DNS query + resolver.ServeDNS(responseWriter, msg) + + // Check if we expect a successful resolution + if !tc.shouldResolve { + if responseMSG == nil || len(responseMSG.Answer) == 0 || responseMSG.Rcode != dns.RcodeSuccess { + // Expected no resolution, test passes + return + } + t.Fatalf("Expected no resolution for %s, but got answer: %v", tc.queryName, responseMSG.Answer) + } + + // Verify we got a successful response + require.NotNil(t, responseMSG, "Should have received a response message") + require.Equal(t, dns.RcodeSuccess, responseMSG.Rcode, "Response should have success status code") + require.Greater(t, len(responseMSG.Answer), 0, "Response should contain at least one answer") + + // Verify the response contains the expected data + answerString := responseMSG.Answer[0].String() + assert.Contains(t, answerString, tc.expectedType, + "Answer should be of type %s, got: %s", tc.expectedType, answerString) + assert.Contains(t, answerString, tc.expectedRData, + "Answer should contain the expected data %s, got: %s", tc.expectedRData, answerString) + }) + } +} diff --git a/client/internal/dns/local_test.go b/client/internal/dns/local_test.go deleted file mode 100644 index 0a42b321a..000000000 --- a/client/internal/dns/local_test.go +++ /dev/null @@ -1,88 +0,0 @@ -package dns - -import ( - "strings" - "testing" - - "github.com/miekg/dns" - - nbdns "github.com/netbirdio/netbird/dns" -) - -func TestLocalResolver_ServeDNS(t *testing.T) { - recordA := nbdns.SimpleRecord{ - Name: "peera.netbird.cloud.", - Type: 1, - Class: nbdns.DefaultClass, - TTL: 300, - RData: "1.2.3.4", - } - - recordCNAME := nbdns.SimpleRecord{ - Name: "peerb.netbird.cloud.", - Type: 5, - Class: nbdns.DefaultClass, - TTL: 300, - RData: "www.netbird.io", - } - - testCases := []struct { - name string - inputRecord nbdns.SimpleRecord - inputMSG *dns.Msg - responseShouldBeNil bool - }{ - { - name: "Should Resolve A Record", - inputRecord: recordA, - inputMSG: new(dns.Msg).SetQuestion(recordA.Name, dns.TypeA), - }, - { - name: "Should Resolve CNAME Record", - inputRecord: recordCNAME, - inputMSG: new(dns.Msg).SetQuestion(recordCNAME.Name, dns.TypeCNAME), - }, - { - name: "Should Not Write When Not Found A Record", - inputRecord: recordA, - inputMSG: new(dns.Msg).SetQuestion("not.found.com", dns.TypeA), - responseShouldBeNil: true, - }, - } - - for _, testCase := range testCases { - t.Run(testCase.name, func(t *testing.T) { - resolver := &localResolver{ - registeredMap: make(registrationMap), - } - _, _ = resolver.registerRecord(testCase.inputRecord) - var responseMSG *dns.Msg - responseWriter := &mockResponseWriter{ - WriteMsgFunc: func(m *dns.Msg) error { - responseMSG = m - return nil - }, - } - - resolver.ServeDNS(responseWriter, testCase.inputMSG) - - if responseMSG == nil || len(responseMSG.Answer) == 0 { - if testCase.responseShouldBeNil { - return - } - t.Fatalf("should write a response message") - } - - answerString := responseMSG.Answer[0].String() - if !strings.Contains(answerString, testCase.inputRecord.Name) { - t.Fatalf("answer doesn't contain the same domain name: \nWant: %s\nGot:%s", testCase.name, answerString) - } - if !strings.Contains(answerString, dns.Type(testCase.inputRecord.Type).String()) { - t.Fatalf("answer doesn't contain the correct type: \nWant: %s\nGot:%s", dns.Type(testCase.inputRecord.Type).String(), answerString) - } - if !strings.Contains(answerString, testCase.inputRecord.RData) { - t.Fatalf("answer doesn't contain the same address: \nWant: %s\nGot:%s", testCase.inputRecord.RData, answerString) - } - }) - } -} diff --git a/client/internal/dns/mock_test.go b/client/internal/dns/mock_test.go deleted file mode 100644 index d52ae24da..000000000 --- a/client/internal/dns/mock_test.go +++ /dev/null @@ -1,26 +0,0 @@ -package dns - -import ( - "net" - - "github.com/miekg/dns" -) - -type mockResponseWriter struct { - WriteMsgFunc func(m *dns.Msg) error -} - -func (rw *mockResponseWriter) WriteMsg(m *dns.Msg) error { - if rw.WriteMsgFunc != nil { - return rw.WriteMsgFunc(m) - } - return nil -} - -func (rw *mockResponseWriter) LocalAddr() net.Addr { return nil } -func (rw *mockResponseWriter) RemoteAddr() net.Addr { return nil } -func (rw *mockResponseWriter) Write([]byte) (int, error) { return 0, nil } -func (rw *mockResponseWriter) Close() error { return nil } -func (rw *mockResponseWriter) TsigStatus() error { return nil } -func (rw *mockResponseWriter) TsigTimersOnly(bool) {} -func (rw *mockResponseWriter) Hijack() {} diff --git a/client/internal/dns/server.go b/client/internal/dns/server.go index 65b90e5f0..3f49c23fd 100644 --- a/client/internal/dns/server.go +++ b/client/internal/dns/server.go @@ -15,6 +15,8 @@ import ( "golang.org/x/exp/maps" "github.com/netbirdio/netbird/client/iface/netstack" + "github.com/netbirdio/netbird/client/internal/dns/local" + "github.com/netbirdio/netbird/client/internal/dns/types" "github.com/netbirdio/netbird/client/internal/listener" "github.com/netbirdio/netbird/client/internal/peer" "github.com/netbirdio/netbird/client/internal/statemanager" @@ -46,8 +48,6 @@ type Server interface { ProbeAvailability() } -type handlerID string - type nsGroupsByDomain struct { domain string groups []*nbdns.NameServerGroup @@ -61,7 +61,7 @@ type DefaultServer struct { mux sync.Mutex service service dnsMuxMap registeredHandlerMap - localResolver *localResolver + localResolver *local.Resolver wgInterface WGIface hostManager hostManager updateSerial uint64 @@ -84,9 +84,9 @@ type DefaultServer struct { type handlerWithStop interface { dns.Handler - stop() - probeAvailability() - id() handlerID + Stop() + ProbeAvailability() + ID() types.HandlerID } type handlerWrapper struct { @@ -95,7 +95,7 @@ type handlerWrapper struct { priority int } -type registeredHandlerMap map[handlerID]handlerWrapper +type registeredHandlerMap map[types.HandlerID]handlerWrapper // NewDefaultServer returns a new dns server func NewDefaultServer( @@ -171,16 +171,14 @@ func newDefaultServer( handlerChain := NewHandlerChain() ctx, stop := context.WithCancel(ctx) defaultServer := &DefaultServer{ - ctx: ctx, - ctxCancel: stop, - disableSys: disableSys, - service: dnsService, - handlerChain: handlerChain, - extraDomains: make(map[domain.Domain]int), - dnsMuxMap: make(registeredHandlerMap), - localResolver: &localResolver{ - registeredMap: make(registrationMap), - }, + ctx: ctx, + ctxCancel: stop, + disableSys: disableSys, + service: dnsService, + handlerChain: handlerChain, + extraDomains: make(map[domain.Domain]int), + dnsMuxMap: make(registeredHandlerMap), + localResolver: local.NewResolver(), wgInterface: wgInterface, statusRecorder: statusRecorder, stateManager: stateManager, @@ -403,7 +401,7 @@ func (s *DefaultServer) ProbeAvailability() { wg.Add(1) go func(mux handlerWithStop) { defer wg.Done() - mux.probeAvailability() + mux.ProbeAvailability() }(mux.handler) } wg.Wait() @@ -420,7 +418,7 @@ func (s *DefaultServer) applyConfiguration(update nbdns.Config) error { s.service.Stop() } - localMuxUpdates, localRecordsByDomain, err := s.buildLocalHandlerUpdate(update.CustomZones) + localMuxUpdates, localRecords, err := s.buildLocalHandlerUpdate(update.CustomZones) if err != nil { return fmt.Errorf("local handler updater: %w", err) } @@ -434,7 +432,7 @@ func (s *DefaultServer) applyConfiguration(update nbdns.Config) error { s.updateMux(muxUpdates) // register local records - s.updateLocalResolver(localRecordsByDomain) + s.localResolver.Update(localRecords) s.currentConfig = dnsConfigToHostDNSConfig(update, s.service.RuntimeIP(), s.service.RuntimePort()) @@ -516,11 +514,9 @@ func (s *DefaultServer) handleErrNoGroupaAll(err error) { ) } -func (s *DefaultServer) buildLocalHandlerUpdate( - customZones []nbdns.CustomZone, -) ([]handlerWrapper, map[string][]nbdns.SimpleRecord, error) { +func (s *DefaultServer) buildLocalHandlerUpdate(customZones []nbdns.CustomZone) ([]handlerWrapper, []nbdns.SimpleRecord, error) { var muxUpdates []handlerWrapper - localRecords := make(map[string][]nbdns.SimpleRecord) + var localRecords []nbdns.SimpleRecord for _, customZone := range customZones { if len(customZone.Records) == 0 { @@ -534,17 +530,13 @@ func (s *DefaultServer) buildLocalHandlerUpdate( priority: PriorityMatchDomain, }) - // group all records under this domain for _, record := range customZone.Records { - var class uint16 = dns.ClassINET if record.Class != nbdns.DefaultClass { log.Warnf("received an invalid class type: %s", record.Class) continue } - - key := buildRecordKey(record.Name, class, uint16(record.Type)) - - localRecords[key] = append(localRecords[key], record) + // zone records contain the fqdn, so we can just flatten them + localRecords = append(localRecords, record) } } @@ -627,7 +619,7 @@ func (s *DefaultServer) createHandlersForDomainGroup(domainGroup nsGroupsByDomai } if len(handler.upstreamServers) == 0 { - handler.stop() + handler.Stop() log.Errorf("received a nameserver group with an invalid nameserver list") continue } @@ -656,7 +648,7 @@ func (s *DefaultServer) updateMux(muxUpdates []handlerWrapper) { // this will introduce a short period of time when the server is not able to handle DNS requests for _, existing := range s.dnsMuxMap { s.deregisterHandler([]string{existing.domain}, existing.priority) - existing.handler.stop() + existing.handler.Stop() } muxUpdateMap := make(registeredHandlerMap) @@ -667,7 +659,7 @@ func (s *DefaultServer) updateMux(muxUpdates []handlerWrapper) { containsRootUpdate = true } s.registerHandler([]string{update.domain}, update.handler, update.priority) - muxUpdateMap[update.handler.id()] = update + muxUpdateMap[update.handler.ID()] = update } // If there's no root update and we had a root handler, restore it @@ -683,33 +675,6 @@ func (s *DefaultServer) updateMux(muxUpdates []handlerWrapper) { s.dnsMuxMap = muxUpdateMap } -func (s *DefaultServer) updateLocalResolver(update map[string][]nbdns.SimpleRecord) { - // remove old records that are no longer present - for key := range s.localResolver.registeredMap { - _, found := update[key] - if !found { - s.localResolver.deleteRecord(key) - } - } - - updatedMap := make(registrationMap) - for _, recs := range update { - for _, rec := range recs { - // convert the record to a dns.RR and register - key, err := s.localResolver.registerRecord(rec) - if err != nil { - log.Warnf("got an error while registering the record (%s), error: %v", - rec.String(), err) - continue - } - - updatedMap[key] = struct{}{} - } - } - - s.localResolver.registeredMap = updatedMap -} - func getNSHostPort(ns nbdns.NameServer) string { return fmt.Sprintf("%s:%d", ns.IP.String(), ns.Port) } diff --git a/client/internal/dns/server_test.go b/client/internal/dns/server_test.go index ed69b0e93..1c7c9b117 100644 --- a/client/internal/dns/server_test.go +++ b/client/internal/dns/server_test.go @@ -23,6 +23,9 @@ import ( "github.com/netbirdio/netbird/client/iface/device" pfmock "github.com/netbirdio/netbird/client/iface/mocks" "github.com/netbirdio/netbird/client/iface/wgaddr" + "github.com/netbirdio/netbird/client/internal/dns/local" + "github.com/netbirdio/netbird/client/internal/dns/test" + "github.com/netbirdio/netbird/client/internal/dns/types" "github.com/netbirdio/netbird/client/internal/netflow" "github.com/netbirdio/netbird/client/internal/peer" "github.com/netbirdio/netbird/client/internal/statemanager" @@ -107,6 +110,7 @@ func generateDummyHandler(domain string, servers []nbdns.NameServer) *upstreamRe } func TestUpdateDNSServer(t *testing.T) { + nameServers := []nbdns.NameServer{ { IP: netip.MustParseAddr("8.8.8.8"), @@ -120,22 +124,21 @@ func TestUpdateDNSServer(t *testing.T) { }, } - dummyHandler := &localResolver{} + dummyHandler := local.NewResolver() testCases := []struct { name string initUpstreamMap registeredHandlerMap - initLocalMap registrationMap + initLocalRecords []nbdns.SimpleRecord initSerial uint64 inputSerial uint64 inputUpdate nbdns.Config shouldFail bool expectedUpstreamMap registeredHandlerMap - expectedLocalMap registrationMap + expectedLocalQs []dns.Question }{ { name: "Initial Config Should Succeed", - initLocalMap: make(registrationMap), initUpstreamMap: make(registeredHandlerMap), initSerial: 0, inputSerial: 1, @@ -159,30 +162,30 @@ func TestUpdateDNSServer(t *testing.T) { }, }, expectedUpstreamMap: registeredHandlerMap{ - generateDummyHandler("netbird.io", nameServers).id(): handlerWrapper{ + generateDummyHandler("netbird.io", nameServers).ID(): handlerWrapper{ domain: "netbird.io", handler: dummyHandler, priority: PriorityMatchDomain, }, - dummyHandler.id(): handlerWrapper{ + dummyHandler.ID(): handlerWrapper{ domain: "netbird.cloud", handler: dummyHandler, priority: PriorityMatchDomain, }, - generateDummyHandler(".", nameServers).id(): handlerWrapper{ + generateDummyHandler(".", nameServers).ID(): handlerWrapper{ domain: nbdns.RootZone, handler: dummyHandler, priority: PriorityDefault, }, }, - expectedLocalMap: registrationMap{buildRecordKey(zoneRecords[0].Name, 1, 1): struct{}{}}, + expectedLocalQs: []dns.Question{{Name: "peera.netbird.cloud.", Qtype: dns.TypeA, Qclass: dns.ClassINET}}, }, { - name: "New Config Should Succeed", - initLocalMap: registrationMap{"netbird.cloud": struct{}{}}, + name: "New Config Should Succeed", + initLocalRecords: []nbdns.SimpleRecord{{Name: "netbird.cloud", Type: 1, Class: nbdns.DefaultClass, TTL: 300, RData: "10.0.0.1"}}, initUpstreamMap: registeredHandlerMap{ - generateDummyHandler(zoneRecords[0].Name, nameServers).id(): handlerWrapper{ - domain: buildRecordKey(zoneRecords[0].Name, 1, 1), + generateDummyHandler(zoneRecords[0].Name, nameServers).ID(): handlerWrapper{ + domain: "netbird.cloud", handler: dummyHandler, priority: PriorityMatchDomain, }, @@ -205,7 +208,7 @@ func TestUpdateDNSServer(t *testing.T) { }, }, expectedUpstreamMap: registeredHandlerMap{ - generateDummyHandler("netbird.io", nameServers).id(): handlerWrapper{ + generateDummyHandler("netbird.io", nameServers).ID(): handlerWrapper{ domain: "netbird.io", handler: dummyHandler, priority: PriorityMatchDomain, @@ -216,22 +219,22 @@ func TestUpdateDNSServer(t *testing.T) { priority: PriorityMatchDomain, }, }, - expectedLocalMap: registrationMap{buildRecordKey(zoneRecords[0].Name, 1, 1): struct{}{}}, + expectedLocalQs: []dns.Question{{Name: zoneRecords[0].Name, Qtype: 1, Qclass: 1}}, }, { - name: "Smaller Config Serial Should Be Skipped", - initLocalMap: make(registrationMap), - initUpstreamMap: make(registeredHandlerMap), - initSerial: 2, - inputSerial: 1, - shouldFail: true, + name: "Smaller Config Serial Should Be Skipped", + initLocalRecords: []nbdns.SimpleRecord{}, + initUpstreamMap: make(registeredHandlerMap), + initSerial: 2, + inputSerial: 1, + shouldFail: true, }, { - name: "Empty NS Group Domain Or Not Primary Element Should Fail", - initLocalMap: make(registrationMap), - initUpstreamMap: make(registeredHandlerMap), - initSerial: 0, - inputSerial: 1, + name: "Empty NS Group Domain Or Not Primary Element Should Fail", + initLocalRecords: []nbdns.SimpleRecord{}, + initUpstreamMap: make(registeredHandlerMap), + initSerial: 0, + inputSerial: 1, inputUpdate: nbdns.Config{ ServiceEnable: true, CustomZones: []nbdns.CustomZone{ @@ -249,11 +252,11 @@ func TestUpdateDNSServer(t *testing.T) { shouldFail: true, }, { - name: "Invalid NS Group Nameservers list Should Fail", - initLocalMap: make(registrationMap), - initUpstreamMap: make(registeredHandlerMap), - initSerial: 0, - inputSerial: 1, + name: "Invalid NS Group Nameservers list Should Fail", + initLocalRecords: []nbdns.SimpleRecord{}, + initUpstreamMap: make(registeredHandlerMap), + initSerial: 0, + inputSerial: 1, inputUpdate: nbdns.Config{ ServiceEnable: true, CustomZones: []nbdns.CustomZone{ @@ -271,11 +274,11 @@ func TestUpdateDNSServer(t *testing.T) { shouldFail: true, }, { - name: "Invalid Custom Zone Records list Should Skip", - initLocalMap: make(registrationMap), - initUpstreamMap: make(registeredHandlerMap), - initSerial: 0, - inputSerial: 1, + name: "Invalid Custom Zone Records list Should Skip", + initLocalRecords: []nbdns.SimpleRecord{}, + initUpstreamMap: make(registeredHandlerMap), + initSerial: 0, + inputSerial: 1, inputUpdate: nbdns.Config{ ServiceEnable: true, CustomZones: []nbdns.CustomZone{ @@ -290,17 +293,17 @@ func TestUpdateDNSServer(t *testing.T) { }, }, }, - expectedUpstreamMap: registeredHandlerMap{generateDummyHandler(".", nameServers).id(): handlerWrapper{ + expectedUpstreamMap: registeredHandlerMap{generateDummyHandler(".", nameServers).ID(): handlerWrapper{ domain: ".", handler: dummyHandler, priority: PriorityDefault, }}, }, { - name: "Empty Config Should Succeed and Clean Maps", - initLocalMap: registrationMap{"netbird.cloud": struct{}{}}, + name: "Empty Config Should Succeed and Clean Maps", + initLocalRecords: []nbdns.SimpleRecord{{Name: "netbird.cloud", Type: int(dns.TypeA), Class: nbdns.DefaultClass, TTL: 300, RData: "10.0.0.1"}}, initUpstreamMap: registeredHandlerMap{ - generateDummyHandler(zoneRecords[0].Name, nameServers).id(): handlerWrapper{ + generateDummyHandler(zoneRecords[0].Name, nameServers).ID(): handlerWrapper{ domain: zoneRecords[0].Name, handler: dummyHandler, priority: PriorityMatchDomain, @@ -310,13 +313,13 @@ func TestUpdateDNSServer(t *testing.T) { inputSerial: 1, inputUpdate: nbdns.Config{ServiceEnable: true}, expectedUpstreamMap: make(registeredHandlerMap), - expectedLocalMap: make(registrationMap), + expectedLocalQs: []dns.Question{}, }, { - name: "Disabled Service Should clean map", - initLocalMap: registrationMap{"netbird.cloud": struct{}{}}, + name: "Disabled Service Should clean map", + initLocalRecords: []nbdns.SimpleRecord{{Name: "netbird.cloud", Type: int(dns.TypeA), Class: nbdns.DefaultClass, TTL: 300, RData: "10.0.0.1"}}, initUpstreamMap: registeredHandlerMap{ - generateDummyHandler(zoneRecords[0].Name, nameServers).id(): handlerWrapper{ + generateDummyHandler(zoneRecords[0].Name, nameServers).ID(): handlerWrapper{ domain: zoneRecords[0].Name, handler: dummyHandler, priority: PriorityMatchDomain, @@ -326,7 +329,7 @@ func TestUpdateDNSServer(t *testing.T) { inputSerial: 1, inputUpdate: nbdns.Config{ServiceEnable: false}, expectedUpstreamMap: make(registeredHandlerMap), - expectedLocalMap: make(registrationMap), + expectedLocalQs: []dns.Question{}, }, } @@ -377,7 +380,7 @@ func TestUpdateDNSServer(t *testing.T) { }() dnsServer.dnsMuxMap = testCase.initUpstreamMap - dnsServer.localResolver.registeredMap = testCase.initLocalMap + dnsServer.localResolver.Update(testCase.initLocalRecords) dnsServer.updateSerial = testCase.initSerial err = dnsServer.UpdateDNSServer(testCase.inputSerial, testCase.inputUpdate) @@ -399,15 +402,23 @@ func TestUpdateDNSServer(t *testing.T) { } } - if len(dnsServer.localResolver.registeredMap) != len(testCase.expectedLocalMap) { - t.Fatalf("update local failed, registered map size is different than expected, want %d, got %d", len(testCase.expectedLocalMap), len(dnsServer.localResolver.registeredMap)) + var responseMSG *dns.Msg + responseWriter := &test.MockResponseWriter{ + WriteMsgFunc: func(m *dns.Msg) error { + responseMSG = m + return nil + }, + } + for _, q := range testCase.expectedLocalQs { + dnsServer.localResolver.ServeDNS(responseWriter, &dns.Msg{ + Question: []dns.Question{q}, + }) } - for key := range testCase.expectedLocalMap { - _, found := dnsServer.localResolver.registeredMap[key] - if !found { - t.Fatalf("update local failed, key %s was not found in the localResolver.registeredMap: %#v", key, dnsServer.localResolver.registeredMap) - } + if len(testCase.expectedLocalQs) > 0 { + assert.NotNil(t, responseMSG, "response message should not be nil") + assert.Equal(t, dns.RcodeSuccess, responseMSG.Rcode, "response code should be success") + assert.NotEmpty(t, responseMSG.Answer, "response message should have answers") } }) } @@ -491,11 +502,12 @@ func TestDNSFakeResolverHandleUpdates(t *testing.T) { dnsServer.dnsMuxMap = registeredHandlerMap{ "id1": handlerWrapper{ domain: zoneRecords[0].Name, - handler: &localResolver{}, + handler: &local.Resolver{}, priority: PriorityMatchDomain, }, } - dnsServer.localResolver.registeredMap = registrationMap{"netbird.cloud": struct{}{}} + //dnsServer.localResolver.RegisteredMap = local.RegistrationMap{local.BuildRecordKey("netbird.cloud", dns.ClassINET, dns.TypeA): struct{}{}} + dnsServer.localResolver.Update([]nbdns.SimpleRecord{{Name: "netbird.cloud", Type: int(dns.TypeA), Class: nbdns.DefaultClass, TTL: 300, RData: "10.0.0.1"}}) dnsServer.updateSerial = 0 nameServers := []nbdns.NameServer{ @@ -582,7 +594,7 @@ func TestDNSServerStartStop(t *testing.T) { } time.Sleep(100 * time.Millisecond) defer dnsServer.Stop() - _, err = dnsServer.localResolver.registerRecord(zoneRecords[0]) + err = dnsServer.localResolver.RegisterRecord(zoneRecords[0]) if err != nil { t.Error(err) } @@ -630,13 +642,11 @@ func TestDNSServerStartStop(t *testing.T) { func TestDNSServerUpstreamDeactivateCallback(t *testing.T) { hostManager := &mockHostConfigurator{} server := DefaultServer{ - ctx: context.Background(), - service: NewServiceViaMemory(&mocWGIface{}), - localResolver: &localResolver{ - registeredMap: make(registrationMap), - }, - handlerChain: NewHandlerChain(), - hostManager: hostManager, + ctx: context.Background(), + service: NewServiceViaMemory(&mocWGIface{}), + localResolver: local.NewResolver(), + handlerChain: NewHandlerChain(), + hostManager: hostManager, currentConfig: HostDNSConfig{ Domains: []DomainConfig{ {false, "domain0", false}, @@ -1004,7 +1014,7 @@ func TestHandlerChain_DomainPriorities(t *testing.T) { t.Run(tc.name, func(t *testing.T) { r := new(dns.Msg) r.SetQuestion(tc.query, dns.TypeA) - w := &ResponseWriterChain{ResponseWriter: &mockResponseWriter{}} + w := &ResponseWriterChain{ResponseWriter: &test.MockResponseWriter{}} if mh, ok := tc.expectedHandler.(*MockHandler); ok { mh.On("ServeDNS", mock.Anything, r).Once() @@ -1037,9 +1047,9 @@ type mockHandler struct { } func (m *mockHandler) ServeDNS(dns.ResponseWriter, *dns.Msg) {} -func (m *mockHandler) stop() {} -func (m *mockHandler) probeAvailability() {} -func (m *mockHandler) id() handlerID { return handlerID(m.Id) } +func (m *mockHandler) Stop() {} +func (m *mockHandler) ProbeAvailability() {} +func (m *mockHandler) ID() types.HandlerID { return types.HandlerID(m.Id) } type mockService struct{} @@ -1113,7 +1123,7 @@ func TestDefaultServer_UpdateMux(t *testing.T) { name string initialHandlers registeredHandlerMap updates []handlerWrapper - expectedHandlers map[string]string // map[handlerID]domain + expectedHandlers map[string]string // map[HandlerID]domain description string }{ { @@ -1409,7 +1419,7 @@ func TestDefaultServer_UpdateMux(t *testing.T) { // Check each expected handler for id, expectedDomain := range tt.expectedHandlers { - handler, exists := server.dnsMuxMap[handlerID(id)] + handler, exists := server.dnsMuxMap[types.HandlerID(id)] assert.True(t, exists, "Expected handler %s not found", id) if exists { assert.Equal(t, expectedDomain, handler.domain, @@ -1418,9 +1428,9 @@ func TestDefaultServer_UpdateMux(t *testing.T) { } // Verify no unexpected handlers exist - for handlerID := range server.dnsMuxMap { - _, expected := tt.expectedHandlers[string(handlerID)] - assert.True(t, expected, "Unexpected handler found: %s", handlerID) + for HandlerID := range server.dnsMuxMap { + _, expected := tt.expectedHandlers[string(HandlerID)] + assert.True(t, expected, "Unexpected handler found: %s", HandlerID) } // Verify the handlerChain state and order @@ -1696,7 +1706,7 @@ func TestExtraDomains(t *testing.T) { handlerChain: NewHandlerChain(), wgInterface: &mocWGIface{}, hostManager: mockHostConfig, - localResolver: &localResolver{}, + localResolver: &local.Resolver{}, service: mockSvc, statusRecorder: peer.NewRecorder("test"), extraDomains: make(map[domain.Domain]int), @@ -1781,7 +1791,7 @@ func TestExtraDomainsRefCounting(t *testing.T) { ctx: context.Background(), handlerChain: NewHandlerChain(), hostManager: mockHostConfig, - localResolver: &localResolver{}, + localResolver: &local.Resolver{}, service: mockSvc, statusRecorder: peer.NewRecorder("test"), extraDomains: make(map[domain.Domain]int), @@ -1833,7 +1843,7 @@ func TestUpdateConfigWithExistingExtraDomains(t *testing.T) { ctx: context.Background(), handlerChain: NewHandlerChain(), hostManager: mockHostConfig, - localResolver: &localResolver{}, + localResolver: &local.Resolver{}, service: mockSvc, statusRecorder: peer.NewRecorder("test"), extraDomains: make(map[domain.Domain]int), @@ -1916,7 +1926,7 @@ func TestDomainCaseHandling(t *testing.T) { ctx: context.Background(), handlerChain: NewHandlerChain(), hostManager: mockHostConfig, - localResolver: &localResolver{}, + localResolver: &local.Resolver{}, service: mockSvc, statusRecorder: peer.NewRecorder("test"), extraDomains: make(map[domain.Domain]int), diff --git a/client/internal/dns/test/mock.go b/client/internal/dns/test/mock.go new file mode 100644 index 000000000..1db452805 --- /dev/null +++ b/client/internal/dns/test/mock.go @@ -0,0 +1,26 @@ +package test + +import ( + "net" + + "github.com/miekg/dns" +) + +type MockResponseWriter struct { + WriteMsgFunc func(m *dns.Msg) error +} + +func (rw *MockResponseWriter) WriteMsg(m *dns.Msg) error { + if rw.WriteMsgFunc != nil { + return rw.WriteMsgFunc(m) + } + return nil +} + +func (rw *MockResponseWriter) LocalAddr() net.Addr { return nil } +func (rw *MockResponseWriter) RemoteAddr() net.Addr { return nil } +func (rw *MockResponseWriter) Write([]byte) (int, error) { return 0, nil } +func (rw *MockResponseWriter) Close() error { return nil } +func (rw *MockResponseWriter) TsigStatus() error { return nil } +func (rw *MockResponseWriter) TsigTimersOnly(bool) {} +func (rw *MockResponseWriter) Hijack() {} diff --git a/client/internal/dns/types/types.go b/client/internal/dns/types/types.go new file mode 100644 index 000000000..5a8be03b7 --- /dev/null +++ b/client/internal/dns/types/types.go @@ -0,0 +1,3 @@ +package types + +type HandlerID string diff --git a/client/internal/dns/upstream.go b/client/internal/dns/upstream.go index fa69d4934..2fbfb3b91 100644 --- a/client/internal/dns/upstream.go +++ b/client/internal/dns/upstream.go @@ -19,6 +19,7 @@ import ( log "github.com/sirupsen/logrus" "github.com/netbirdio/netbird/client/iface" + "github.com/netbirdio/netbird/client/internal/dns/types" "github.com/netbirdio/netbird/client/internal/peer" "github.com/netbirdio/netbird/client/proto" ) @@ -81,21 +82,21 @@ func (u *upstreamResolverBase) String() string { } // ID returns the unique handler ID -func (u *upstreamResolverBase) id() handlerID { +func (u *upstreamResolverBase) ID() types.HandlerID { servers := slices.Clone(u.upstreamServers) slices.Sort(servers) hash := sha256.New() hash.Write([]byte(u.domain + ":")) hash.Write([]byte(strings.Join(servers, ","))) - return handlerID("upstream-" + hex.EncodeToString(hash.Sum(nil)[:8])) + return types.HandlerID("upstream-" + hex.EncodeToString(hash.Sum(nil)[:8])) } func (u *upstreamResolverBase) MatchSubdomains() bool { return true } -func (u *upstreamResolverBase) stop() { +func (u *upstreamResolverBase) Stop() { log.Debugf("stopping serving DNS for upstreams %s", u.upstreamServers) u.cancel() } @@ -198,9 +199,9 @@ func (u *upstreamResolverBase) checkUpstreamFails(err error) { ) } -// probeAvailability tests all upstream servers simultaneously and +// ProbeAvailability tests all upstream servers simultaneously and // disables the resolver if none work -func (u *upstreamResolverBase) probeAvailability() { +func (u *upstreamResolverBase) ProbeAvailability() { u.mutex.Lock() defer u.mutex.Unlock() diff --git a/client/internal/dns/upstream_test.go b/client/internal/dns/upstream_test.go index 5dbcc9f79..13bc91a37 100644 --- a/client/internal/dns/upstream_test.go +++ b/client/internal/dns/upstream_test.go @@ -8,6 +8,8 @@ import ( "time" "github.com/miekg/dns" + + "github.com/netbirdio/netbird/client/internal/dns/test" ) func TestUpstreamResolver_ServeDNS(t *testing.T) { @@ -66,7 +68,7 @@ func TestUpstreamResolver_ServeDNS(t *testing.T) { } var responseMSG *dns.Msg - responseWriter := &mockResponseWriter{ + responseWriter := &test.MockResponseWriter{ WriteMsgFunc: func(m *dns.Msg) error { responseMSG = m return nil @@ -130,7 +132,7 @@ func TestUpstreamResolver_DeactivationReactivation(t *testing.T) { resolver.failsTillDeact = 0 resolver.reactivatePeriod = time.Microsecond * 100 - responseWriter := &mockResponseWriter{ + responseWriter := &test.MockResponseWriter{ WriteMsgFunc: func(m *dns.Msg) error { return nil }, } diff --git a/client/internal/dnsfwd/forwarder.go b/client/internal/dnsfwd/forwarder.go index 8f6a31f47..45b479632 100644 --- a/client/internal/dnsfwd/forwarder.go +++ b/client/internal/dnsfwd/forwarder.go @@ -33,6 +33,8 @@ type DNSForwarder struct { dnsServer *dns.Server mux *dns.ServeMux + tcpServer *dns.Server + tcpMux *dns.ServeMux mutex sync.RWMutex fwdEntries []*ForwarderEntry @@ -50,22 +52,41 @@ func NewDNSForwarder(listenAddress string, ttl uint32, firewall firewall.Manager } func (f *DNSForwarder) Listen(entries []*ForwarderEntry) error { - log.Infof("listen DNS forwarder on address=%s", f.listenAddress) - mux := dns.NewServeMux() + log.Infof("starting DNS forwarder on address=%s", f.listenAddress) - dnsServer := &dns.Server{ + // UDP server + mux := dns.NewServeMux() + f.mux = mux + f.dnsServer = &dns.Server{ Addr: f.listenAddress, Net: "udp", Handler: mux, } - f.dnsServer = dnsServer - f.mux = mux + // TCP server + tcpMux := dns.NewServeMux() + f.tcpMux = tcpMux + f.tcpServer = &dns.Server{ + Addr: f.listenAddress, + Net: "tcp", + Handler: tcpMux, + } f.UpdateDomains(entries) - return dnsServer.ListenAndServe() -} + errCh := make(chan error, 2) + go func() { + log.Infof("DNS UDP listener running on %s", f.listenAddress) + errCh <- f.dnsServer.ListenAndServe() + }() + go func() { + log.Infof("DNS TCP listener running on %s", f.listenAddress) + errCh <- f.tcpServer.ListenAndServe() + }() + + // return the first error we get (e.g. bind failure or shutdown) + return <-errCh +} func (f *DNSForwarder) UpdateDomains(entries []*ForwarderEntry) { f.mutex.Lock() defer f.mutex.Unlock() @@ -77,31 +98,41 @@ func (f *DNSForwarder) UpdateDomains(entries []*ForwarderEntry) { } oldDomains := filterDomains(f.fwdEntries) - for _, d := range oldDomains { f.mux.HandleRemove(d.PunycodeString()) + f.tcpMux.HandleRemove(d.PunycodeString()) } newDomains := filterDomains(entries) for _, d := range newDomains { - f.mux.HandleFunc(d.PunycodeString(), f.handleDNSQuery) + f.mux.HandleFunc(d.PunycodeString(), f.handleDNSQueryUDP) + f.tcpMux.HandleFunc(d.PunycodeString(), f.handleDNSQueryTCP) } f.fwdEntries = entries - log.Debugf("Updated domains from %v to %v", oldDomains, newDomains) } func (f *DNSForwarder) Close(ctx context.Context) error { - if f.dnsServer == nil { - return nil + var result *multierror.Error + + if f.dnsServer != nil { + if err := f.dnsServer.ShutdownContext(ctx); err != nil { + result = multierror.Append(result, fmt.Errorf("UDP shutdown: %w", err)) + } } - return f.dnsServer.ShutdownContext(ctx) + if f.tcpServer != nil { + if err := f.tcpServer.ShutdownContext(ctx); err != nil { + result = multierror.Append(result, fmt.Errorf("TCP shutdown: %w", err)) + } + } + + return nberrors.FormatErrorOrNil(result) } -func (f *DNSForwarder) handleDNSQuery(w dns.ResponseWriter, query *dns.Msg) { +func (f *DNSForwarder) handleDNSQuery(w dns.ResponseWriter, query *dns.Msg) *dns.Msg { if len(query.Question) == 0 { - return + return nil } question := query.Question[0] log.Tracef("received DNS request for DNS forwarder: domain=%v type=%v class=%v", @@ -123,20 +154,53 @@ func (f *DNSForwarder) handleDNSQuery(w dns.ResponseWriter, query *dns.Msg) { if err := w.WriteMsg(resp); err != nil { log.Errorf("failed to write DNS response: %v", err) } - return + return nil } ctx, cancel := context.WithTimeout(context.Background(), upstreamTimeout) defer cancel() ips, err := net.DefaultResolver.LookupNetIP(ctx, network, domain) if err != nil { - f.handleDNSError(w, resp, domain, err) - return + f.handleDNSError(w, query, resp, domain, err) + return nil } f.updateInternalState(domain, ips) f.addIPsToResponse(resp, domain, ips) + return resp +} + +func (f *DNSForwarder) handleDNSQueryUDP(w dns.ResponseWriter, query *dns.Msg) { + + resp := f.handleDNSQuery(w, query) + if resp == nil { + return + } + + opt := query.IsEdns0() + maxSize := dns.MinMsgSize + if opt != nil { + // client advertised a larger EDNS0 buffer + maxSize = int(opt.UDPSize()) + } + + // if our response is too big, truncate and set the TC bit + if resp.Len() > maxSize { + resp.Truncate(maxSize) + } + + if err := w.WriteMsg(resp); err != nil { + log.Errorf("failed to write DNS response: %v", err) + } +} + +func (f *DNSForwarder) handleDNSQueryTCP(w dns.ResponseWriter, query *dns.Msg) { + resp := f.handleDNSQuery(w, query) + if resp == nil { + return + } + if err := w.WriteMsg(resp); err != nil { log.Errorf("failed to write DNS response: %v", err) } @@ -179,7 +243,7 @@ func (f *DNSForwarder) updateFirewall(matchingEntries []*ForwarderEntry, prefixe } // handleDNSError processes DNS lookup errors and sends an appropriate error response -func (f *DNSForwarder) handleDNSError(w dns.ResponseWriter, resp *dns.Msg, domain string, err error) { +func (f *DNSForwarder) handleDNSError(w dns.ResponseWriter, query, resp *dns.Msg, domain string, err error) { var dnsErr *net.DNSError switch { @@ -191,7 +255,7 @@ func (f *DNSForwarder) handleDNSError(w dns.ResponseWriter, resp *dns.Msg, domai } if dnsErr.Server != "" { - log.Warnf("failed to resolve query for domain=%s server=%s: %v", domain, dnsErr.Server, err) + log.Warnf("failed to resolve query for type=%s domain=%s server=%s: %v", dns.TypeToString[query.Question[0].Qtype], domain, dnsErr.Server, err) } else { log.Warnf(errResolveFailed, domain, err) } diff --git a/client/internal/dnsfwd/manager.go b/client/internal/dnsfwd/manager.go index e4a23450f..91abce823 100644 --- a/client/internal/dnsfwd/manager.go +++ b/client/internal/dnsfwd/manager.go @@ -33,6 +33,7 @@ type Manager struct { statusRecorder *peer.Status fwRules []firewall.Rule + tcpRules []firewall.Rule dnsForwarder *DNSForwarder } @@ -107,6 +108,13 @@ func (m *Manager) allowDNSFirewall() error { } m.fwRules = dnsRules + tcpRules, err := m.firewall.AddPeerFiltering(nil, net.IP{0, 0, 0, 0}, firewall.ProtocolTCP, nil, dport, firewall.ActionAccept, "") + if err != nil { + log.Errorf("failed to add allow DNS router rules, err: %v", err) + return err + } + m.tcpRules = tcpRules + return nil } @@ -117,7 +125,13 @@ func (m *Manager) dropDNSFirewall() error { mErr = multierror.Append(mErr, fmt.Errorf("failed to delete DNS router rules, err: %v", err)) } } + for _, rule := range m.tcpRules { + if err := m.firewall.DeletePeerRule(rule); err != nil { + mErr = multierror.Append(mErr, fmt.Errorf("failed to delete DNS router rules, err: %v", err)) + } + } m.fwRules = nil + m.tcpRules = nil return nberrors.FormatErrorOrNil(mErr) } diff --git a/client/internal/networkmonitor/check_change_bsd.go b/client/internal/networkmonitor/check_change_bsd.go index bb327a877..f5eb2c739 100644 --- a/client/internal/networkmonitor/check_change_bsd.go +++ b/client/internal/networkmonitor/check_change_bsd.go @@ -19,7 +19,7 @@ import ( func checkChange(ctx context.Context, nexthopv4, nexthopv6 systemops.Nexthop) error { fd, err := unix.Socket(syscall.AF_ROUTE, syscall.SOCK_RAW, syscall.AF_UNSPEC) if err != nil { - return fmt.Errorf("failed to open routing socket: %v", err) + return fmt.Errorf("open routing socket: %v", err) } defer func() { err := unix.Close(fd) diff --git a/client/internal/networkmonitor/check_change_windows.go b/client/internal/networkmonitor/check_change_windows.go index 582865738..814584863 100644 --- a/client/internal/networkmonitor/check_change_windows.go +++ b/client/internal/networkmonitor/check_change_windows.go @@ -13,7 +13,7 @@ import ( func checkChange(ctx context.Context, nexthopv4, nexthopv6 systemops.Nexthop) error { routeMonitor, err := systemops.NewRouteMonitor(ctx) if err != nil { - return fmt.Errorf("failed to create route monitor: %w", err) + return fmt.Errorf("create route monitor: %w", err) } defer func() { if err := routeMonitor.Stop(); err != nil { @@ -38,35 +38,49 @@ func checkChange(ctx context.Context, nexthopv4, nexthopv6 systemops.Nexthop) er } func routeChanged(route systemops.RouteUpdate, nexthopv4, nexthopv6 systemops.Nexthop) bool { - intf := "" - if route.Interface != nil { - intf = route.Interface.Name - if isSoftInterface(intf) { - log.Debugf("Network monitor: ignoring default route change for soft interface %s", intf) - return false - } + if intf := route.NextHop.Intf; intf != nil && isSoftInterface(intf.Name) { + log.Debugf("Network monitor: ignoring default route change for next hop with soft interface %s", route.NextHop) + return false + } + + // TODO: for the empty nexthop ip (on-link), determine the family differently + nexthop := nexthopv4 + if route.NextHop.IP.Is6() { + nexthop = nexthopv6 } switch route.Type { - case systemops.RouteModified: - // TODO: get routing table to figure out if our route is affected for modified routes - log.Infof("Network monitor: default route changed: via %s, interface %s", route.NextHop, intf) - return true - case systemops.RouteAdded: - if route.NextHop.Is4() && route.NextHop != nexthopv4.IP || route.NextHop.Is6() && route.NextHop != nexthopv6.IP { - log.Infof("Network monitor: default route added: via %s, interface %s", route.NextHop, intf) - return true - } + case systemops.RouteModified, systemops.RouteAdded: + return handleRouteAddedOrModified(route, nexthop) case systemops.RouteDeleted: - if nexthopv4.Intf != nil && route.NextHop == nexthopv4.IP || nexthopv6.Intf != nil && route.NextHop == nexthopv6.IP { - log.Infof("Network monitor: default route removed: via %s, interface %s", route.NextHop, intf) - return true - } + return handleRouteDeleted(route, nexthop) } return false } +func handleRouteAddedOrModified(route systemops.RouteUpdate, nexthop systemops.Nexthop) bool { + // For added/modified routes, we care about different next hops + if !nexthop.Equal(route.NextHop) { + action := "changed" + if route.Type == systemops.RouteAdded { + action = "added" + } + log.Infof("Network monitor: default route %s: via %s", action, route.NextHop) + return true + } + return false +} + +func handleRouteDeleted(route systemops.RouteUpdate, nexthop systemops.Nexthop) bool { + // For deleted routes, we care about our tracked next hop being deleted + if nexthop.Equal(route.NextHop) { + log.Infof("Network monitor: default route removed: via %s", route.NextHop) + return true + } + return false +} + func isSoftInterface(name string) bool { return strings.Contains(strings.ToLower(name), "isatap") || strings.Contains(strings.ToLower(name), "teredo") } diff --git a/client/internal/networkmonitor/check_change_windows_test.go b/client/internal/networkmonitor/check_change_windows_test.go new file mode 100644 index 000000000..29ff34dca --- /dev/null +++ b/client/internal/networkmonitor/check_change_windows_test.go @@ -0,0 +1,404 @@ +package networkmonitor + +import ( + "net" + "net/netip" + "testing" + + "github.com/stretchr/testify/assert" + + "github.com/netbirdio/netbird/client/internal/routemanager/systemops" +) + +func TestRouteChanged(t *testing.T) { + tests := []struct { + name string + route systemops.RouteUpdate + nexthopv4 systemops.Nexthop + nexthopv6 systemops.Nexthop + expected bool + }{ + { + name: "soft interface should be ignored", + route: systemops.RouteUpdate{ + Type: systemops.RouteModified, + Destination: netip.PrefixFrom(netip.IPv4Unspecified(), 0), + NextHop: systemops.Nexthop{ + IP: netip.MustParseAddr("192.168.1.1"), + Intf: &net.Interface{ + Name: "ISATAP-Interface", // isSoftInterface checks name + }, + }, + }, + nexthopv4: systemops.Nexthop{ + IP: netip.MustParseAddr("192.168.1.2"), + }, + nexthopv6: systemops.Nexthop{ + IP: netip.MustParseAddr("2001:db8::1"), + }, + expected: false, + }, + { + name: "modified route with different v4 nexthop IP should return true", + route: systemops.RouteUpdate{ + Type: systemops.RouteModified, + Destination: netip.PrefixFrom(netip.IPv4Unspecified(), 0), + NextHop: systemops.Nexthop{ + IP: netip.MustParseAddr("192.168.1.1"), + Intf: &net.Interface{ + Index: 1, Name: "eth0", + }, + }, + }, + nexthopv4: systemops.Nexthop{ + IP: netip.MustParseAddr("192.168.1.2"), + Intf: &net.Interface{ + Index: 1, Name: "eth0", + }, + }, + nexthopv6: systemops.Nexthop{ + IP: netip.MustParseAddr("2001:db8::1"), + }, + expected: true, + }, + { + name: "modified route with same v4 nexthop (IP and Intf Index) should return false", + route: systemops.RouteUpdate{ + Type: systemops.RouteModified, + Destination: netip.PrefixFrom(netip.IPv4Unspecified(), 0), + NextHop: systemops.Nexthop{ + IP: netip.MustParseAddr("192.168.1.1"), + Intf: &net.Interface{ + Index: 1, Name: "eth0", + }, + }, + }, + nexthopv4: systemops.Nexthop{ + IP: netip.MustParseAddr("192.168.1.1"), + Intf: &net.Interface{ + Index: 1, Name: "eth0", + }, + }, + nexthopv6: systemops.Nexthop{ + IP: netip.MustParseAddr("2001:db8::1"), + }, + expected: false, + }, + { + name: "added route with different v6 nexthop IP should return true", + route: systemops.RouteUpdate{ + Type: systemops.RouteAdded, + Destination: netip.PrefixFrom(netip.IPv6Unspecified(), 0), + NextHop: systemops.Nexthop{ + IP: netip.MustParseAddr("2001:db8::2"), + Intf: &net.Interface{ + Index: 1, Name: "eth0", + }, + }, + }, + nexthopv4: systemops.Nexthop{ + IP: netip.MustParseAddr("192.168.1.1"), + }, + nexthopv6: systemops.Nexthop{ + IP: netip.MustParseAddr("2001:db8::1"), + Intf: &net.Interface{ + Index: 1, Name: "eth0", + }, + }, + expected: true, + }, + { + name: "added route with same v6 nexthop (IP and Intf Index) should return false", + route: systemops.RouteUpdate{ + Type: systemops.RouteAdded, + Destination: netip.PrefixFrom(netip.IPv6Unspecified(), 0), + NextHop: systemops.Nexthop{ + IP: netip.MustParseAddr("2001:db8::1"), + Intf: &net.Interface{ + Index: 1, Name: "eth0", + }, + }, + }, + nexthopv4: systemops.Nexthop{ + IP: netip.MustParseAddr("192.168.1.1"), + }, + nexthopv6: systemops.Nexthop{ + IP: netip.MustParseAddr("2001:db8::1"), + Intf: &net.Interface{ + Index: 1, Name: "eth0", + }, + }, + expected: false, + }, + { + name: "deleted route matching tracked v4 nexthop (IP and Intf Index) should return true", + route: systemops.RouteUpdate{ + Type: systemops.RouteDeleted, + Destination: netip.PrefixFrom(netip.IPv4Unspecified(), 0), + NextHop: systemops.Nexthop{ + IP: netip.MustParseAddr("192.168.1.1"), + Intf: &net.Interface{ + Index: 1, Name: "eth0", + }, + }, + }, + nexthopv4: systemops.Nexthop{ + IP: netip.MustParseAddr("192.168.1.1"), + Intf: &net.Interface{ + Index: 1, Name: "eth0", + }, + }, + nexthopv6: systemops.Nexthop{ + IP: netip.MustParseAddr("2001:db8::1"), + }, + expected: true, + }, + { + name: "deleted route not matching tracked v4 nexthop (different IP) should return false", + route: systemops.RouteUpdate{ + Type: systemops.RouteDeleted, + Destination: netip.PrefixFrom(netip.IPv4Unspecified(), 0), + NextHop: systemops.Nexthop{ + IP: netip.MustParseAddr("192.168.1.3"), // Different IP + Intf: &net.Interface{ + Index: 1, Name: "eth0", + }, + }, + }, + nexthopv4: systemops.Nexthop{ + IP: netip.MustParseAddr("192.168.1.1"), + Intf: &net.Interface{ + Index: 1, Name: "eth0", + }, + }, + nexthopv6: systemops.Nexthop{ + IP: netip.MustParseAddr("2001:db8::1"), + }, + expected: false, + }, + { + name: "modified v4 route with same IP, different Intf Index should return true", + route: systemops.RouteUpdate{ + Type: systemops.RouteModified, + Destination: netip.PrefixFrom(netip.IPv4Unspecified(), 0), + NextHop: systemops.Nexthop{ + IP: netip.MustParseAddr("192.168.1.1"), + Intf: &net.Interface{Index: 2, Name: "eth1"}, // Different Intf Index + }, + }, + nexthopv4: systemops.Nexthop{ + IP: netip.MustParseAddr("192.168.1.1"), + Intf: &net.Interface{Index: 1, Name: "eth0"}, + }, + expected: true, + }, + { + name: "modified v4 route with same IP, one Intf nil, other non-nil should return true", + route: systemops.RouteUpdate{ + Type: systemops.RouteModified, + Destination: netip.PrefixFrom(netip.IPv4Unspecified(), 0), + NextHop: systemops.Nexthop{ + IP: netip.MustParseAddr("192.168.1.1"), + Intf: nil, // Intf is nil + }, + }, + nexthopv4: systemops.Nexthop{ + IP: netip.MustParseAddr("192.168.1.1"), + Intf: &net.Interface{Index: 1, Name: "eth0"}, // Tracked Intf is not nil + }, + expected: true, + }, + { + name: "added v4 route with same IP, different Intf Index should return true", + route: systemops.RouteUpdate{ + Type: systemops.RouteAdded, + Destination: netip.PrefixFrom(netip.IPv4Unspecified(), 0), + NextHop: systemops.Nexthop{ + IP: netip.MustParseAddr("192.168.1.1"), + Intf: &net.Interface{Index: 2, Name: "eth1"}, // Different Intf Index + }, + }, + nexthopv4: systemops.Nexthop{ + IP: netip.MustParseAddr("192.168.1.1"), + Intf: &net.Interface{Index: 1, Name: "eth0"}, + }, + expected: true, + }, + { + name: "deleted v4 route with same IP, different Intf Index should return false", + route: systemops.RouteUpdate{ + Type: systemops.RouteDeleted, + Destination: netip.PrefixFrom(netip.IPv4Unspecified(), 0), + NextHop: systemops.Nexthop{ // This is the route being deleted + IP: netip.MustParseAddr("192.168.1.1"), + Intf: &net.Interface{Index: 1, Name: "eth0"}, + }, + }, + nexthopv4: systemops.Nexthop{ // This is our tracked nexthop + IP: netip.MustParseAddr("192.168.1.1"), + Intf: &net.Interface{Index: 2, Name: "eth1"}, // Different Intf Index + }, + expected: false, // Because nexthopv4.Equal(route.NextHop) will be false + }, + { + name: "modified v6 route with different IP, same Intf Index should return true", + route: systemops.RouteUpdate{ + Type: systemops.RouteModified, + Destination: netip.PrefixFrom(netip.IPv6Unspecified(), 0), + NextHop: systemops.Nexthop{ + IP: netip.MustParseAddr("2001:db8::3"), // Different IP + Intf: &net.Interface{Index: 1, Name: "eth0"}, + }, + }, + nexthopv6: systemops.Nexthop{ + IP: netip.MustParseAddr("2001:db8::1"), + Intf: &net.Interface{Index: 1, Name: "eth0"}, + }, + expected: true, + }, + { + name: "modified v6 route with same IP, different Intf Index should return true", + route: systemops.RouteUpdate{ + Type: systemops.RouteModified, + Destination: netip.PrefixFrom(netip.IPv6Unspecified(), 0), + NextHop: systemops.Nexthop{ + IP: netip.MustParseAddr("2001:db8::1"), + Intf: &net.Interface{Index: 2, Name: "eth1"}, // Different Intf Index + }, + }, + nexthopv6: systemops.Nexthop{ + IP: netip.MustParseAddr("2001:db8::1"), + Intf: &net.Interface{Index: 1, Name: "eth0"}, + }, + expected: true, + }, + { + name: "modified v6 route with same IP, same Intf Index should return false", + route: systemops.RouteUpdate{ + Type: systemops.RouteModified, + Destination: netip.PrefixFrom(netip.IPv6Unspecified(), 0), + NextHop: systemops.Nexthop{ + IP: netip.MustParseAddr("2001:db8::1"), + Intf: &net.Interface{Index: 1, Name: "eth0"}, + }, + }, + nexthopv6: systemops.Nexthop{ + IP: netip.MustParseAddr("2001:db8::1"), + Intf: &net.Interface{Index: 1, Name: "eth0"}, + }, + expected: false, + }, + { + name: "deleted v6 route matching tracked nexthop (IP and Intf Index) should return true", + route: systemops.RouteUpdate{ + Type: systemops.RouteDeleted, + Destination: netip.PrefixFrom(netip.IPv6Unspecified(), 0), + NextHop: systemops.Nexthop{ + IP: netip.MustParseAddr("2001:db8::1"), + Intf: &net.Interface{Index: 1, Name: "eth0"}, + }, + }, + nexthopv6: systemops.Nexthop{ + IP: netip.MustParseAddr("2001:db8::1"), + Intf: &net.Interface{Index: 1, Name: "eth0"}, + }, + expected: true, + }, + { + name: "deleted v6 route not matching tracked nexthop (different IP) should return false", + route: systemops.RouteUpdate{ + Type: systemops.RouteDeleted, + Destination: netip.PrefixFrom(netip.IPv6Unspecified(), 0), + NextHop: systemops.Nexthop{ + IP: netip.MustParseAddr("2001:db8::3"), // Different IP + Intf: &net.Interface{Index: 1, Name: "eth0"}, + }, + }, + nexthopv6: systemops.Nexthop{ + IP: netip.MustParseAddr("2001:db8::1"), + Intf: &net.Interface{Index: 1, Name: "eth0"}, + }, + expected: false, + }, + { + name: "deleted v6 route not matching tracked nexthop (same IP, different Intf Index) should return false", + route: systemops.RouteUpdate{ + Type: systemops.RouteDeleted, + Destination: netip.PrefixFrom(netip.IPv6Unspecified(), 0), + NextHop: systemops.Nexthop{ // This is the route being deleted + IP: netip.MustParseAddr("2001:db8::1"), + Intf: &net.Interface{Index: 1, Name: "eth0"}, + }, + }, + nexthopv6: systemops.Nexthop{ // This is our tracked nexthop + IP: netip.MustParseAddr("2001:db8::1"), + Intf: &net.Interface{Index: 2, Name: "eth1"}, // Different Intf Index + }, + expected: false, + }, + { + name: "unknown route type should return false", + route: systemops.RouteUpdate{ + Type: systemops.RouteUpdateType(99), // Unknown type + Destination: netip.PrefixFrom(netip.IPv4Unspecified(), 0), + NextHop: systemops.Nexthop{ + IP: netip.MustParseAddr("192.168.1.1"), + Intf: &net.Interface{Index: 1, Name: "eth0"}, + }, + }, + nexthopv4: systemops.Nexthop{ + IP: netip.MustParseAddr("192.168.1.2"), // Different from route.NextHop + Intf: &net.Interface{Index: 1, Name: "eth0"}, + }, + expected: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := routeChanged(tt.route, tt.nexthopv4, tt.nexthopv6) + assert.Equal(t, tt.expected, result) + }) + } +} + +func TestIsSoftInterface(t *testing.T) { + tests := []struct { + name string + ifname string + expected bool + }{ + { + name: "ISATAP interface should be detected", + ifname: "ISATAP tunnel adapter", + expected: true, + }, + { + name: "lowercase soft interface should be detected", + ifname: "isatap.{14A5CF17-CA72-43EC-B4EA-B4B093641B7D}", + expected: true, + }, + { + name: "Teredo interface should be detected", + ifname: "Teredo Tunneling Pseudo-Interface", + expected: true, + }, + { + name: "regular interface should not be detected as soft", + ifname: "eth0", + expected: false, + }, + { + name: "another regular interface should not be detected as soft", + ifname: "wlan0", + expected: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := isSoftInterface(tt.ifname) + assert.Equal(t, tt.expected, result) + }) + } +} diff --git a/client/internal/networkmonitor/monitor.go b/client/internal/networkmonitor/monitor.go index 5896b66b6..accdd9c9d 100644 --- a/client/internal/networkmonitor/monitor.go +++ b/client/internal/networkmonitor/monitor.go @@ -118,9 +118,12 @@ func (nw *NetworkMonitor) Stop() { } func (nw *NetworkMonitor) checkChanges(ctx context.Context, event chan struct{}, nexthop4 systemops.Nexthop, nexthop6 systemops.Nexthop) { + defer close(event) for { if err := checkChangeFn(ctx, nexthop4, nexthop6); err != nil { - close(event) + if !errors.Is(err, context.Canceled) { + log.Errorf("Network monitor: failed to check for changes: %v", err) + } return } // prevent blocking diff --git a/client/internal/routemanager/notifier/notifier.go b/client/internal/routemanager/notifier/notifier.go index ebdd60323..25a3a71e0 100644 --- a/client/internal/routemanager/notifier/notifier.go +++ b/client/internal/routemanager/notifier/notifier.go @@ -32,6 +32,10 @@ func (n *Notifier) SetListener(listener listener.NetworkChangeListener) { func (n *Notifier) SetInitialClientRoutes(clientRoutes []*route.Route) { nets := make([]string, 0) for _, r := range clientRoutes { + // filter out domain routes + if r.IsDynamic() { + continue + } nets = append(nets, r.Network.String()) } sort.Strings(nets) diff --git a/client/internal/routemanager/systemops/systemops.go b/client/internal/routemanager/systemops/systemops.go index 5c117b94d..fd511fc20 100644 --- a/client/internal/routemanager/systemops/systemops.go +++ b/client/internal/routemanager/systemops/systemops.go @@ -1,6 +1,7 @@ package systemops import ( + "fmt" "net" "net/netip" "sync" @@ -15,6 +16,20 @@ type Nexthop struct { Intf *net.Interface } +// Equal checks if two nexthops are equal. +func (n Nexthop) Equal(other Nexthop) bool { + return n.IP == other.IP && (n.Intf == nil && other.Intf == nil || + n.Intf != nil && other.Intf != nil && n.Intf.Index == other.Intf.Index) +} + +// String returns a string representation of the nexthop. +func (n Nexthop) String() string { + if n.Intf == nil { + return n.IP.String() + } + return fmt.Sprintf("%s @ %d (%s)", n.IP.String(), n.Intf.Index, n.Intf.Name) +} + type ExclusionCounter = refcounter.Counter[netip.Prefix, struct{}, Nexthop] type SysOps struct { diff --git a/client/internal/routemanager/systemops/systemops_windows.go b/client/internal/routemanager/systemops/systemops_windows.go index ad325e123..f66161595 100644 --- a/client/internal/routemanager/systemops/systemops_windows.go +++ b/client/internal/routemanager/systemops/systemops_windows.go @@ -33,8 +33,7 @@ type RouteUpdateType int type RouteUpdate struct { Type RouteUpdateType Destination netip.Prefix - NextHop netip.Addr - Interface *net.Interface + NextHop Nexthop } // RouteMonitor provides a way to monitor changes in the routing table. @@ -231,15 +230,15 @@ func (rm *RouteMonitor) parseUpdate(row *MIB_IPFORWARD_ROW2, notificationType MI intf, err := net.InterfaceByIndex(idx) if err != nil { log.Warnf("failed to get interface name for index %d: %v", idx, err) - update.Interface = &net.Interface{ + update.NextHop.Intf = &net.Interface{ Index: idx, } } else { - update.Interface = intf + update.NextHop.Intf = intf } } - log.Tracef("Received route update with destination %v, next hop %v, interface %v", row.DestinationPrefix, row.NextHop, update.Interface) + log.Tracef("Received route update with destination %v, next hop %v, interface %v", row.DestinationPrefix, row.NextHop, update.NextHop.Intf) dest := parseIPPrefix(row.DestinationPrefix, idx) if !dest.Addr().IsValid() { return RouteUpdate{}, fmt.Errorf("invalid destination: %v", row) @@ -262,7 +261,7 @@ func (rm *RouteMonitor) parseUpdate(row *MIB_IPFORWARD_ROW2, notificationType MI update.Type = updateType update.Destination = dest - update.NextHop = nexthop + update.NextHop.IP = nexthop return update, nil } diff --git a/client/server/debug.go b/client/server/debug.go index b42b1467a..7de3e8609 100644 --- a/client/server/debug.go +++ b/client/server/debug.go @@ -51,14 +51,16 @@ func (s *Server) DebugBundle(_ context.Context, req *proto.DebugBundleRequest) ( } if req.GetUploadURL() == "" { - return &proto.DebugBundleResponse{Path: path}, nil } key, err := uploadDebugBundle(context.Background(), req.GetUploadURL(), s.config.ManagementURL.String(), path) if err != nil { + log.Errorf("failed to upload debug bundle to %s: %v", req.GetUploadURL(), err) return &proto.DebugBundleResponse{Path: path, UploadFailureReason: err.Error()}, nil } + log.Infof("debug bundle uploaded to %s with key %s", req.GetUploadURL(), key) + return &proto.DebugBundleResponse{Path: path, UploadedKey: key}, nil } diff --git a/client/ui/client_ui.go b/client/ui/client_ui.go index d0b1bacf6..2c8023185 100644 --- a/client/ui/client_ui.go +++ b/client/ui/client_ui.go @@ -51,14 +51,17 @@ const ( ) func main() { - daemonAddr, showSettings, showNetworks, errorMsg, saveLogsInFile := parseFlags() + daemonAddr, showSettings, showNetworks, showDebug, errorMsg, saveLogsInFile := parseFlags() // Initialize file logging if needed. + var logFile string if saveLogsInFile { - if err := initLogFile(); err != nil { + file, err := initLogFile() + if err != nil { log.Errorf("error while initializing log: %v", err) return } + logFile = file } // Create the Fyne application. @@ -72,13 +75,13 @@ func main() { } // Create the service client (this also builds the settings or networks UI if requested). - client := newServiceClient(daemonAddr, a, showSettings, showNetworks) + client := newServiceClient(daemonAddr, logFile, a, showSettings, showNetworks, showDebug) // Watch for theme/settings changes to update the icon. go watchSettingsChanges(a, client) // Run in window mode if any UI flag was set. - if showSettings || showNetworks { + if showSettings || showNetworks || showDebug { a.Run() return } @@ -99,7 +102,7 @@ func main() { } // parseFlags reads and returns all needed command-line flags. -func parseFlags() (daemonAddr string, showSettings, showNetworks bool, errorMsg string, saveLogsInFile bool) { +func parseFlags() (daemonAddr string, showSettings, showNetworks, showDebug bool, errorMsg string, saveLogsInFile bool) { defaultDaemonAddr := "unix:///var/run/netbird.sock" if runtime.GOOS == "windows" { defaultDaemonAddr = "tcp://127.0.0.1:41731" @@ -107,25 +110,17 @@ func parseFlags() (daemonAddr string, showSettings, showNetworks bool, errorMsg flag.StringVar(&daemonAddr, "daemon-addr", defaultDaemonAddr, "Daemon service address to serve CLI requests [unix|tcp]://[path|host:port]") flag.BoolVar(&showSettings, "settings", false, "run settings window") flag.BoolVar(&showNetworks, "networks", false, "run networks window") + flag.BoolVar(&showDebug, "debug", false, "run debug window") flag.StringVar(&errorMsg, "error-msg", "", "displays an error message window") - - tmpDir := "/tmp" - if runtime.GOOS == "windows" { - tmpDir = os.TempDir() - } - flag.BoolVar(&saveLogsInFile, "use-log-file", false, fmt.Sprintf("save logs in a file: %s/netbird-ui-PID.log", tmpDir)) + flag.BoolVar(&saveLogsInFile, "use-log-file", false, fmt.Sprintf("save logs in a file: %s/netbird-ui-PID.log", os.TempDir())) flag.Parse() return } // initLogFile initializes logging into a file. -func initLogFile() error { - tmpDir := "/tmp" - if runtime.GOOS == "windows" { - tmpDir = os.TempDir() - } - logFile := path.Join(tmpDir, fmt.Sprintf("netbird-ui-%d.log", os.Getpid())) - return util.InitLog("trace", logFile) +func initLogFile() (string, error) { + logFile := path.Join(os.TempDir(), fmt.Sprintf("netbird-ui-%d.log", os.Getpid())) + return logFile, util.InitLog("trace", logFile) } // watchSettingsChanges listens for Fyne theme/settings changes and updates the client icon. @@ -168,9 +163,10 @@ var iconConnectingMacOS []byte var iconErrorMacOS []byte type serviceClient struct { - ctx context.Context - addr string - conn proto.DaemonServiceClient + ctx context.Context + cancel context.CancelFunc + addr string + conn proto.DaemonServiceClient icAbout []byte icConnected []byte @@ -231,13 +227,14 @@ type serviceClient struct { daemonVersion string updateIndicationLock sync.Mutex isUpdateIconActive bool - showRoutes bool - wRoutes fyne.Window + showNetworks bool + wNetworks fyne.Window eventManager *event.Manager exitNodeMu sync.Mutex mExitNodeItems []menuHandler + logFile string } type menuHandler struct { @@ -248,25 +245,30 @@ type menuHandler struct { // newServiceClient instance constructor // // This constructor also builds the UI elements for the settings window. -func newServiceClient(addr string, a fyne.App, showSettings bool, showRoutes bool) *serviceClient { +func newServiceClient(addr string, logFile string, a fyne.App, showSettings bool, showNetworks bool, showDebug bool) *serviceClient { + ctx, cancel := context.WithCancel(context.Background()) s := &serviceClient{ - ctx: context.Background(), + ctx: ctx, + cancel: cancel, addr: addr, app: a, + logFile: logFile, sendNotification: false, showAdvancedSettings: showSettings, - showRoutes: showRoutes, + showNetworks: showNetworks, update: version.NewUpdate(), } s.setNewIcons() - if showSettings { + switch { + case showSettings: s.showSettingsUI() - return s - } else if showRoutes { + case showNetworks: s.showNetworksUI() + case showDebug: + s.showDebugUI() } return s @@ -313,6 +315,8 @@ func (s *serviceClient) updateIcon() { func (s *serviceClient) showSettingsUI() { // add settings window UI elements. s.wSettings = s.app.NewWindow("NetBird Settings") + s.wSettings.SetOnClosed(s.cancel) + s.iMngURL = widget.NewEntry() s.iAdminURL = widget.NewEntry() s.iConfigFile = widget.NewEntry() @@ -743,11 +747,10 @@ func (s *serviceClient) onTrayReady() { s.runSelfCommand("settings", "true") }() case <-s.mCreateDebugBundle.ClickedCh: + s.mCreateDebugBundle.Disable() go func() { - if err := s.createAndOpenDebugBundle(); err != nil { - log.Errorf("Failed to create debug bundle: %v", err) - s.app.SendNotification(fyne.NewNotification("Error", "Failed to create debug bundle")) - } + defer s.mCreateDebugBundle.Enable() + s.runSelfCommand("debug", "true") }() case <-s.mQuit.ClickedCh: systray.Quit() @@ -789,7 +792,7 @@ func (s *serviceClient) onTrayReady() { func (s *serviceClient) runSelfCommand(command, arg string) { proc, err := os.Executable() if err != nil { - log.Errorf("show %s failed with error: %v", command, err) + log.Errorf("Error getting executable path: %v", err) return } @@ -798,14 +801,48 @@ func (s *serviceClient) runSelfCommand(command, arg string) { fmt.Sprintf("--daemon-addr=%s", s.addr), ) - out, err := cmd.CombinedOutput() - if exitErr, ok := err.(*exec.ExitError); ok && exitErr.ExitCode() == 1 { - log.Errorf("start %s UI: %v, %s", command, err, string(out)) + if out := s.attachOutput(cmd); out != nil { + defer func() { + if err := out.Close(); err != nil { + log.Errorf("Error closing log file %s: %v", s.logFile, err) + } + }() + } + + log.Printf("Running command: %s --%s=%s --daemon-addr=%s", proc, command, arg, s.addr) + + err = cmd.Run() + + if err != nil { + var exitErr *exec.ExitError + if errors.As(err, &exitErr) { + log.Printf("Command '%s %s' failed with exit code %d", command, arg, exitErr.ExitCode()) + } else { + log.Printf("Failed to start/run command '%s %s': %v", command, arg, err) + } return } - if len(out) != 0 { - log.Infof("command %s executed: %s", command, string(out)) + + log.Printf("Command '%s %s' completed successfully.", command, arg) +} + +func (s *serviceClient) attachOutput(cmd *exec.Cmd) *os.File { + if s.logFile == "" { + // attach child's streams to parent's streams + cmd.Stdout = os.Stdout + cmd.Stderr = os.Stderr + + return nil } + + out, err := os.OpenFile(s.logFile, os.O_WRONLY|os.O_APPEND, 0) + if err != nil { + log.Errorf("Failed to open log file %s: %v", s.logFile, err) + return nil + } + cmd.Stdout = out + cmd.Stderr = out + return out } func normalizedVersion(version string) string { @@ -818,9 +855,7 @@ func normalizedVersion(version string) string { // onTrayExit is called when the tray icon is closed. func (s *serviceClient) onTrayExit() { - for _, item := range s.mExitNodeItems { - item.cancel() - } + s.cancel() } // getSrvClient connection to the service. @@ -829,7 +864,7 @@ func (s *serviceClient) getSrvClient(timeout time.Duration) (proto.DaemonService return s.conn, nil } - ctx, cancel := context.WithTimeout(context.Background(), timeout) + ctx, cancel := context.WithTimeout(s.ctx, timeout) defer cancel() conn, err := grpc.DialContext( diff --git a/client/ui/debug.go b/client/ui/debug.go index 845ea284c..ab7dba37a 100644 --- a/client/ui/debug.go +++ b/client/ui/debug.go @@ -3,48 +3,721 @@ package main import ( + "context" "fmt" "path/filepath" + "strconv" + "sync" + "time" "fyne.io/fyne/v2" + "fyne.io/fyne/v2/container" + "fyne.io/fyne/v2/dialog" + "fyne.io/fyne/v2/widget" + log "github.com/sirupsen/logrus" "github.com/skratchdot/open-golang/open" + "github.com/netbirdio/netbird/client/internal" "github.com/netbirdio/netbird/client/proto" nbstatus "github.com/netbirdio/netbird/client/status" + uptypes "github.com/netbirdio/netbird/upload-server/types" ) -func (s *serviceClient) createAndOpenDebugBundle() error { +// Initial state for the debug collection +type debugInitialState struct { + wasDown bool + logLevel proto.LogLevel + isLevelTrace bool +} + +// Debug collection parameters +type debugCollectionParams struct { + duration time.Duration + anonymize bool + systemInfo bool + upload bool + uploadURL string + enablePersistence bool +} + +// UI components for progress tracking +type progressUI struct { + statusLabel *widget.Label + progressBar *widget.ProgressBar + uiControls []fyne.Disableable + window fyne.Window +} + +func (s *serviceClient) showDebugUI() { + w := s.app.NewWindow("NetBird Debug") + w.SetOnClosed(s.cancel) + + w.Resize(fyne.NewSize(600, 500)) + w.SetFixedSize(true) + + anonymizeCheck := widget.NewCheck("Anonymize sensitive information (public IPs, domains, ...)", nil) + systemInfoCheck := widget.NewCheck("Include system information (routes, interfaces, ...)", nil) + systemInfoCheck.SetChecked(true) + uploadCheck := widget.NewCheck("Upload bundle automatically after creation", nil) + uploadCheck.SetChecked(true) + + uploadURLLabel := widget.NewLabel("Debug upload URL:") + uploadURL := widget.NewEntry() + uploadURL.SetText(uptypes.DefaultBundleURL) + uploadURL.SetPlaceHolder("Enter upload URL") + + uploadURLContainer := container.NewVBox( + uploadURLLabel, + uploadURL, + ) + + uploadCheck.OnChanged = func(checked bool) { + if checked { + uploadURLContainer.Show() + } else { + uploadURLContainer.Hide() + } + } + + debugModeContainer := container.NewHBox() + runForDurationCheck := widget.NewCheck("Run with trace logs before creating bundle", nil) + runForDurationCheck.SetChecked(true) + + forLabel := widget.NewLabel("for") + + durationInput := widget.NewEntry() + durationInput.SetText("1") + minutesLabel := widget.NewLabel("minute") + durationInput.Validator = func(s string) error { + return validateMinute(s, minutesLabel) + } + + noteLabel := widget.NewLabel("Note: NetBird will be brought up and down during collection") + + runForDurationCheck.OnChanged = func(checked bool) { + if checked { + forLabel.Show() + durationInput.Show() + minutesLabel.Show() + noteLabel.Show() + } else { + forLabel.Hide() + durationInput.Hide() + minutesLabel.Hide() + noteLabel.Hide() + } + } + + debugModeContainer.Add(runForDurationCheck) + debugModeContainer.Add(forLabel) + debugModeContainer.Add(durationInput) + debugModeContainer.Add(minutesLabel) + + statusLabel := widget.NewLabel("") + statusLabel.Hide() + + progressBar := widget.NewProgressBar() + progressBar.Hide() + + createButton := widget.NewButton("Create Debug Bundle", nil) + + // UI controls that should be disabled during debug collection + uiControls := []fyne.Disableable{ + anonymizeCheck, + systemInfoCheck, + uploadCheck, + uploadURL, + runForDurationCheck, + durationInput, + createButton, + } + + createButton.OnTapped = s.getCreateHandler( + statusLabel, + progressBar, + uploadCheck, + uploadURL, + anonymizeCheck, + systemInfoCheck, + runForDurationCheck, + durationInput, + uiControls, + w, + ) + + content := container.NewVBox( + widget.NewLabel("Create a debug bundle to help troubleshoot issues with NetBird"), + widget.NewLabel(""), + anonymizeCheck, + systemInfoCheck, + uploadCheck, + uploadURLContainer, + widget.NewLabel(""), + debugModeContainer, + noteLabel, + widget.NewLabel(""), + statusLabel, + progressBar, + createButton, + ) + + paddedContent := container.NewPadded(content) + w.SetContent(paddedContent) + + w.Show() +} + +func validateMinute(s string, minutesLabel *widget.Label) error { + if val, err := strconv.Atoi(s); err != nil || val < 1 { + return fmt.Errorf("must be a number ≥ 1") + } + if s == "1" { + minutesLabel.SetText("minute") + } else { + minutesLabel.SetText("minutes") + } + return nil +} + +// disableUIControls disables the provided UI controls +func disableUIControls(controls []fyne.Disableable) { + for _, control := range controls { + control.Disable() + } +} + +// enableUIControls enables the provided UI controls +func enableUIControls(controls []fyne.Disableable) { + for _, control := range controls { + control.Enable() + } +} + +func (s *serviceClient) getCreateHandler( + statusLabel *widget.Label, + progressBar *widget.ProgressBar, + uploadCheck *widget.Check, + uploadURL *widget.Entry, + anonymizeCheck *widget.Check, + systemInfoCheck *widget.Check, + runForDurationCheck *widget.Check, + duration *widget.Entry, + uiControls []fyne.Disableable, + w fyne.Window, +) func() { + return func() { + disableUIControls(uiControls) + statusLabel.Show() + + var url string + if uploadCheck.Checked { + url = uploadURL.Text + if url == "" { + statusLabel.SetText("Error: Upload URL is required when upload is enabled") + enableUIControls(uiControls) + return + } + } + + params := &debugCollectionParams{ + anonymize: anonymizeCheck.Checked, + systemInfo: systemInfoCheck.Checked, + upload: uploadCheck.Checked, + uploadURL: url, + enablePersistence: true, + } + + runForDuration := runForDurationCheck.Checked + if runForDuration { + minutes, err := time.ParseDuration(duration.Text + "m") + if err != nil { + statusLabel.SetText(fmt.Sprintf("Error: Invalid duration: %v", err)) + enableUIControls(uiControls) + return + } + params.duration = minutes + + statusLabel.SetText(fmt.Sprintf("Running in debug mode for %d minutes...", int(minutes.Minutes()))) + progressBar.Show() + progressBar.SetValue(0) + + go s.handleRunForDuration( + statusLabel, + progressBar, + uiControls, + w, + params, + ) + return + } + + statusLabel.SetText("Creating debug bundle...") + go s.handleDebugCreation( + anonymizeCheck.Checked, + systemInfoCheck.Checked, + uploadCheck.Checked, + url, + statusLabel, + uiControls, + w, + ) + } +} + +func (s *serviceClient) handleRunForDuration( + statusLabel *widget.Label, + progressBar *widget.ProgressBar, + uiControls []fyne.Disableable, + w fyne.Window, + params *debugCollectionParams, +) { + progressUI := &progressUI{ + statusLabel: statusLabel, + progressBar: progressBar, + uiControls: uiControls, + window: w, + } + conn, err := s.getSrvClient(failFastTimeout) if err != nil { - return fmt.Errorf("get client: %v", err) + handleError(progressUI, fmt.Sprintf("Failed to get client for debug: %v", err)) + return + } + + initialState, err := s.getInitialState(conn) + if err != nil { + handleError(progressUI, err.Error()) + return + } + + statusOutput, err := s.collectDebugData(conn, initialState, params, progressUI) + if err != nil { + handleError(progressUI, err.Error()) + return + } + + if err := s.createDebugBundleFromCollection(conn, params, statusOutput, progressUI); err != nil { + handleError(progressUI, err.Error()) + return + } + + s.restoreServiceState(conn, initialState) + + progressUI.statusLabel.SetText("Bundle created successfully") +} + +// Get initial state of the service +func (s *serviceClient) getInitialState(conn proto.DaemonServiceClient) (*debugInitialState, error) { + statusResp, err := conn.Status(s.ctx, &proto.StatusRequest{}) + if err != nil { + return nil, fmt.Errorf(" get status: %v", err) + } + + logLevelResp, err := conn.GetLogLevel(s.ctx, &proto.GetLogLevelRequest{}) + if err != nil { + return nil, fmt.Errorf("get log level: %v", err) + } + + wasDown := statusResp.Status != string(internal.StatusConnected) && + statusResp.Status != string(internal.StatusConnecting) + + initialLogLevel := logLevelResp.GetLevel() + initialLevelTrace := initialLogLevel >= proto.LogLevel_TRACE + + return &debugInitialState{ + wasDown: wasDown, + logLevel: initialLogLevel, + isLevelTrace: initialLevelTrace, + }, nil +} + +// Handle progress tracking during collection +func startProgressTracker(ctx context.Context, wg *sync.WaitGroup, duration time.Duration, progress *progressUI) { + progress.progressBar.Show() + progress.progressBar.SetValue(0) + + startTime := time.Now() + endTime := startTime.Add(duration) + wg.Add(1) + + go func() { + defer wg.Done() + ticker := time.NewTicker(500 * time.Millisecond) + defer ticker.Stop() + + for { + select { + case <-ctx.Done(): + return + case <-ticker.C: + remaining := time.Until(endTime) + if remaining <= 0 { + remaining = 0 + } + + elapsed := time.Since(startTime) + progressVal := float64(elapsed) / float64(duration) + if progressVal > 1.0 { + progressVal = 1.0 + } + + progress.progressBar.SetValue(progressVal) + progress.statusLabel.SetText(fmt.Sprintf("Running with trace logs... %s remaining", formatDuration(remaining))) + } + } + }() + +} + +func (s *serviceClient) configureServiceForDebug( + conn proto.DaemonServiceClient, + state *debugInitialState, + enablePersistence bool, +) error { + if state.wasDown { + if _, err := conn.Up(s.ctx, &proto.UpRequest{}); err != nil { + return fmt.Errorf("bring service up: %v", err) + } + log.Info("Service brought up for debug") + time.Sleep(time.Second * 10) + } + + if !state.isLevelTrace { + if _, err := conn.SetLogLevel(s.ctx, &proto.SetLogLevelRequest{Level: proto.LogLevel_TRACE}); err != nil { + return fmt.Errorf("set log level to TRACE: %v", err) + } + log.Info("Log level set to TRACE for debug") + } + + if _, err := conn.Down(s.ctx, &proto.DownRequest{}); err != nil { + return fmt.Errorf("bring service down: %v", err) + } + time.Sleep(time.Second) + + if enablePersistence { + if _, err := conn.SetNetworkMapPersistence(s.ctx, &proto.SetNetworkMapPersistenceRequest{ + Enabled: true, + }); err != nil { + return fmt.Errorf("enable network map persistence: %v", err) + } + log.Info("Network map persistence enabled for debug") + } + + if _, err := conn.Up(s.ctx, &proto.UpRequest{}); err != nil { + return fmt.Errorf("bring service back up: %v", err) + } + time.Sleep(time.Second * 3) + + return nil +} + +func (s *serviceClient) collectDebugData( + conn proto.DaemonServiceClient, + state *debugInitialState, + params *debugCollectionParams, + progress *progressUI, +) (string, error) { + ctx, cancel := context.WithTimeout(s.ctx, params.duration) + defer cancel() + var wg sync.WaitGroup + startProgressTracker(ctx, &wg, params.duration, progress) + + if err := s.configureServiceForDebug(conn, state, params.enablePersistence); err != nil { + return "", err + } + + postUpStatus, err := conn.Status(s.ctx, &proto.StatusRequest{GetFullPeerStatus: true}) + if err != nil { + log.Warnf("Failed to get post-up status: %v", err) + } + + var postUpStatusOutput string + if postUpStatus != nil { + overview := nbstatus.ConvertToStatusOutputOverview(postUpStatus, params.anonymize, "", nil, nil, nil) + postUpStatusOutput = nbstatus.ParseToFullDetailSummary(overview) + } + headerPostUp := fmt.Sprintf("----- NetBird post-up - Timestamp: %s", time.Now().Format(time.RFC3339)) + statusOutput := fmt.Sprintf("%s\n%s", headerPostUp, postUpStatusOutput) + + wg.Wait() + progress.progressBar.Hide() + progress.statusLabel.SetText("Collecting debug data...") + + preDownStatus, err := conn.Status(s.ctx, &proto.StatusRequest{GetFullPeerStatus: true}) + if err != nil { + log.Warnf("Failed to get pre-down status: %v", err) + } + + var preDownStatusOutput string + if preDownStatus != nil { + overview := nbstatus.ConvertToStatusOutputOverview(preDownStatus, params.anonymize, "", nil, nil, nil) + preDownStatusOutput = nbstatus.ParseToFullDetailSummary(overview) + } + headerPreDown := fmt.Sprintf("----- NetBird pre-down - Timestamp: %s - Duration: %s", + time.Now().Format(time.RFC3339), params.duration) + statusOutput = fmt.Sprintf("%s\n%s\n%s", statusOutput, headerPreDown, preDownStatusOutput) + + return statusOutput, nil +} + +// Create the debug bundle with collected data +func (s *serviceClient) createDebugBundleFromCollection( + conn proto.DaemonServiceClient, + params *debugCollectionParams, + statusOutput string, + progress *progressUI, +) error { + progress.statusLabel.SetText("Creating debug bundle with collected logs...") + + request := &proto.DebugBundleRequest{ + Anonymize: params.anonymize, + Status: statusOutput, + SystemInfo: params.systemInfo, + } + + if params.upload { + request.UploadURL = params.uploadURL + } + + resp, err := conn.DebugBundle(s.ctx, request) + if err != nil { + return fmt.Errorf("create debug bundle: %v", err) + } + + // Show appropriate dialog based on upload status + localPath := resp.GetPath() + uploadFailureReason := resp.GetUploadFailureReason() + uploadedKey := resp.GetUploadedKey() + + if params.upload { + if uploadFailureReason != "" { + showUploadFailedDialog(progress.window, localPath, uploadFailureReason) + } else { + showUploadSuccessDialog(progress.window, localPath, uploadedKey) + } + } else { + showBundleCreatedDialog(progress.window, localPath) + } + + enableUIControls(progress.uiControls) + return nil +} + +// Restore service to original state +func (s *serviceClient) restoreServiceState(conn proto.DaemonServiceClient, state *debugInitialState) { + if state.wasDown { + if _, err := conn.Down(s.ctx, &proto.DownRequest{}); err != nil { + log.Errorf("Failed to restore down state: %v", err) + } else { + log.Info("Service state restored to down") + } + } + + if !state.isLevelTrace { + if _, err := conn.SetLogLevel(s.ctx, &proto.SetLogLevelRequest{Level: state.logLevel}); err != nil { + log.Errorf("Failed to restore log level: %v", err) + } else { + log.Info("Log level restored to original setting") + } + } +} + +// Handle errors during debug collection +func handleError(progress *progressUI, errMsg string) { + log.Errorf("%s", errMsg) + progress.statusLabel.SetText(errMsg) + progress.progressBar.Hide() + enableUIControls(progress.uiControls) +} + +func (s *serviceClient) handleDebugCreation( + anonymize bool, + systemInfo bool, + upload bool, + uploadURL string, + statusLabel *widget.Label, + uiControls []fyne.Disableable, + w fyne.Window, +) { + log.Infof("Creating debug bundle (Anonymized: %v, System Info: %v, Upload Attempt: %v)...", + anonymize, systemInfo, upload) + + resp, err := s.createDebugBundle(anonymize, systemInfo, uploadURL) + if err != nil { + log.Errorf("Failed to create debug bundle: %v", err) + statusLabel.SetText(fmt.Sprintf("Error creating bundle: %v", err)) + enableUIControls(uiControls) + return + } + + localPath := resp.GetPath() + uploadFailureReason := resp.GetUploadFailureReason() + uploadedKey := resp.GetUploadedKey() + + if upload { + if uploadFailureReason != "" { + showUploadFailedDialog(w, localPath, uploadFailureReason) + } else { + showUploadSuccessDialog(w, localPath, uploadedKey) + } + } else { + showBundleCreatedDialog(w, localPath) + } + + enableUIControls(uiControls) + statusLabel.SetText("Bundle created successfully") +} + +func (s *serviceClient) createDebugBundle(anonymize bool, systemInfo bool, uploadURL string) (*proto.DebugBundleResponse, error) { + conn, err := s.getSrvClient(failFastTimeout) + if err != nil { + return nil, fmt.Errorf("get client: %v", err) } statusResp, err := conn.Status(s.ctx, &proto.StatusRequest{GetFullPeerStatus: true}) if err != nil { - return fmt.Errorf("failed to get status: %v", err) + log.Warnf("failed to get status for debug bundle: %v", err) } - overview := nbstatus.ConvertToStatusOutputOverview(statusResp, true, "", nil, nil, nil) - statusOutput := nbstatus.ParseToFullDetailSummary(overview) + var statusOutput string + if statusResp != nil { + overview := nbstatus.ConvertToStatusOutputOverview(statusResp, anonymize, "", nil, nil, nil) + statusOutput = nbstatus.ParseToFullDetailSummary(overview) + } - resp, err := conn.DebugBundle(s.ctx, &proto.DebugBundleRequest{ - Anonymize: true, + request := &proto.DebugBundleRequest{ + Anonymize: anonymize, Status: statusOutput, - SystemInfo: true, - }) + SystemInfo: systemInfo, + } + + if uploadURL != "" { + request.UploadURL = uploadURL + } + + resp, err := conn.DebugBundle(s.ctx, request) if err != nil { - return fmt.Errorf("failed to create debug bundle: %v", err) + return nil, fmt.Errorf("failed to create debug bundle via daemon: %v", err) } - bundleDir := filepath.Dir(resp.GetPath()) - if err := open.Start(bundleDir); err != nil { - return fmt.Errorf("failed to open debug bundle directory: %v", err) - } - - s.app.SendNotification(fyne.NewNotification( - "Debug Bundle", - fmt.Sprintf("Debug bundle created at %s. Administrator privileges are required to access it.", resp.GetPath()), - )) - - return nil + return resp, nil +} + +// formatDuration formats a duration in HH:MM:SS format +func formatDuration(d time.Duration) string { + d = d.Round(time.Second) + h := d / time.Hour + d %= time.Hour + m := d / time.Minute + d %= time.Minute + s := d / time.Second + return fmt.Sprintf("%02d:%02d:%02d", h, m, s) +} + +// createButtonWithAction creates a button with the given label and action +func createButtonWithAction(label string, action func()) *widget.Button { + button := widget.NewButton(label, action) + return button +} + +// showUploadFailedDialog displays a dialog when upload fails +func showUploadFailedDialog(w fyne.Window, localPath, failureReason string) { + content := container.NewVBox( + widget.NewLabel(fmt.Sprintf("Bundle upload failed:\n%s\n\n"+ + "A local copy was saved at:\n%s", failureReason, localPath)), + ) + + customDialog := dialog.NewCustom("Upload Failed", "Cancel", content, w) + + buttonBox := container.NewHBox( + createButtonWithAction("Open file", func() { + log.Infof("Attempting to open local file: %s", localPath) + if openErr := open.Start(localPath); openErr != nil { + log.Errorf("Failed to open local file '%s': %v", localPath, openErr) + dialog.ShowError(fmt.Errorf("open the local file:\n%s\n\nError: %v", localPath, openErr), w) + } + }), + createButtonWithAction("Open folder", func() { + folderPath := filepath.Dir(localPath) + log.Infof("Attempting to open local folder: %s", folderPath) + if openErr := open.Start(folderPath); openErr != nil { + log.Errorf("Failed to open local folder '%s': %v", folderPath, openErr) + dialog.ShowError(fmt.Errorf("open the local folder:\n%s\n\nError: %v", folderPath, openErr), w) + } + }), + ) + + content.Add(buttonBox) + customDialog.Show() +} + +// showUploadSuccessDialog displays a dialog when upload succeeds +func showUploadSuccessDialog(w fyne.Window, localPath, uploadedKey string) { + log.Infof("Upload key: %s", uploadedKey) + keyEntry := widget.NewEntry() + keyEntry.SetText(uploadedKey) + keyEntry.Disable() + + content := container.NewVBox( + widget.NewLabel("Bundle uploaded successfully!"), + widget.NewLabel(""), + widget.NewLabel("Upload key:"), + keyEntry, + widget.NewLabel(""), + widget.NewLabel(fmt.Sprintf("Local copy saved at:\n%s", localPath)), + ) + + customDialog := dialog.NewCustom("Upload Successful", "OK", content, w) + + copyBtn := createButtonWithAction("Copy key", func() { + w.Clipboard().SetContent(uploadedKey) + log.Info("Upload key copied to clipboard") + }) + + buttonBox := createButtonBox(localPath, w, copyBtn) + content.Add(buttonBox) + customDialog.Show() +} + +// showBundleCreatedDialog displays a dialog when bundle is created without upload +func showBundleCreatedDialog(w fyne.Window, localPath string) { + content := container.NewVBox( + widget.NewLabel(fmt.Sprintf("Bundle created locally at:\n%s\n\n"+ + "Administrator privileges may be required to access the file.", localPath)), + ) + + customDialog := dialog.NewCustom("Debug Bundle Created", "Cancel", content, w) + + buttonBox := createButtonBox(localPath, w, nil) + content.Add(buttonBox) + customDialog.Show() +} + +func createButtonBox(localPath string, w fyne.Window, elems ...fyne.Widget) *fyne.Container { + box := container.NewHBox() + for _, elem := range elems { + box.Add(elem) + } + + fileBtn := createButtonWithAction("Open file", func() { + log.Infof("Attempting to open local file: %s", localPath) + if openErr := open.Start(localPath); openErr != nil { + log.Errorf("Failed to open local file '%s': %v", localPath, openErr) + dialog.ShowError(fmt.Errorf("open the local file:\n%s\n\nError: %v", localPath, openErr), w) + } + }) + + folderBtn := createButtonWithAction("Open folder", func() { + folderPath := filepath.Dir(localPath) + log.Infof("Attempting to open local folder: %s", folderPath) + if openErr := open.Start(folderPath); openErr != nil { + log.Errorf("Failed to open local folder '%s': %v", folderPath, openErr) + dialog.ShowError(fmt.Errorf("open the local folder:\n%s\n\nError: %v", folderPath, openErr), w) + } + }) + + box.Add(fileBtn) + box.Add(folderBtn) + + return box } diff --git a/client/ui/network.go b/client/ui/network.go index ddd8d5000..435917f30 100644 --- a/client/ui/network.go +++ b/client/ui/network.go @@ -34,7 +34,8 @@ const ( type filter string func (s *serviceClient) showNetworksUI() { - s.wRoutes = s.app.NewWindow("Networks") + s.wNetworks = s.app.NewWindow("Networks") + s.wNetworks.SetOnClosed(s.cancel) allGrid := container.New(layout.NewGridLayout(3)) go s.updateNetworks(allGrid, allNetworks) @@ -78,8 +79,8 @@ func (s *serviceClient) showNetworksUI() { content := container.NewBorder(nil, buttonBox, nil, nil, scrollContainer) - s.wRoutes.SetContent(content) - s.wRoutes.Show() + s.wNetworks.SetContent(content) + s.wNetworks.Show() s.startAutoRefresh(10*time.Second, tabs, allGrid, overlappingGrid, exitNodeGrid) } @@ -148,7 +149,7 @@ func (s *serviceClient) updateNetworks(grid *fyne.Container, f filter) { grid.Add(resolvedIPsSelector) } - s.wRoutes.Content().Refresh() + s.wNetworks.Content().Refresh() grid.Refresh() } @@ -305,7 +306,7 @@ func (s *serviceClient) getNetworksRequest(f filter, appendRoute bool) *proto.Se func (s *serviceClient) showError(err error) { wrappedMessage := wrapText(err.Error(), 50) - dialog.ShowError(fmt.Errorf("%s", wrappedMessage), s.wRoutes) + dialog.ShowError(fmt.Errorf("%s", wrappedMessage), s.wNetworks) } func (s *serviceClient) startAutoRefresh(interval time.Duration, tabs *container.AppTabs, allGrid, overlappingGrid, exitNodesGrid *fyne.Container) { @@ -316,14 +317,15 @@ func (s *serviceClient) startAutoRefresh(interval time.Duration, tabs *container } }() - s.wRoutes.SetOnClosed(func() { + s.wNetworks.SetOnClosed(func() { ticker.Stop() + s.cancel() }) } func (s *serviceClient) updateNetworksBasedOnDisplayTab(tabs *container.AppTabs, allGrid, overlappingGrid, exitNodesGrid *fyne.Container) { grid, f := getGridAndFilterFromTab(tabs, allGrid, overlappingGrid, exitNodesGrid) - s.wRoutes.Content().Refresh() + s.wNetworks.Content().Refresh() s.updateNetworks(grid, f) } @@ -373,7 +375,7 @@ func (s *serviceClient) recreateExitNodeMenu(exitNodes []*proto.Network) { node.Selected, ) - ctx, cancel := context.WithCancel(context.Background()) + ctx, cancel := context.WithCancel(s.ctx) s.mExitNodeItems = append(s.mExitNodeItems, menuHandler{ MenuItem: menuItem, cancel: cancel, diff --git a/dns/dns.go b/dns/dns.go index 3a1c76e56..f889a32ec 100644 --- a/dns/dns.go +++ b/dns/dns.go @@ -66,17 +66,17 @@ func (s SimpleRecord) String() string { func (s SimpleRecord) Len() uint16 { emptyString := s.RData == "" switch s.Type { - case 1: + case int(dns.TypeA): if emptyString { return 0 } return net.IPv4len - case 5: + case int(dns.TypeCNAME): if emptyString || s.RData == "." { return 1 } return uint16(len(s.RData) + 1) - case 28: + case int(dns.TypeAAAA): if emptyString { return 0 } diff --git a/management/client/grpc.go b/management/client/grpc.go index 956aaebb2..2f4729e23 100644 --- a/management/client/grpc.go +++ b/management/client/grpc.go @@ -128,13 +128,7 @@ func (c *GrpcClient) Sync(ctx context.Context, sysInfo *system.Info, msgHandler return err } - streamErr := c.handleStream(ctx, *serverPubKey, sysInfo, msgHandler) - if c.conn.GetState() != connectivity.Shutdown { - if err := c.conn.Close(); err != nil { - log.Warnf("failed closing connection to Management service: %s", err) - } - } - return streamErr + return c.handleStream(ctx, *serverPubKey, sysInfo, msgHandler) } err := backoff.Retry(operation, defaultBackoff(ctx)) diff --git a/management/client/rest/accounts.go b/management/client/rest/accounts.go index f38b19f70..29d4ac79d 100644 --- a/management/client/rest/accounts.go +++ b/management/client/rest/accounts.go @@ -20,7 +20,9 @@ func (a *AccountsAPI) List(ctx context.Context) ([]api.Account, error) { if err != nil { return nil, err } - defer resp.Body.Close() + if resp.Body != nil { + defer resp.Body.Close() + } ret, err := parseResponse[[]api.Account](resp) return ret, err } @@ -36,7 +38,9 @@ func (a *AccountsAPI) Update(ctx context.Context, accountID string, request api. if err != nil { return nil, err } - defer resp.Body.Close() + if resp.Body != nil { + defer resp.Body.Close() + } ret, err := parseResponse[api.Account](resp) return &ret, err } @@ -48,7 +52,9 @@ func (a *AccountsAPI) Delete(ctx context.Context, accountID string) error { if err != nil { return err } - defer resp.Body.Close() + if resp.Body != nil { + defer resp.Body.Close() + } return nil } diff --git a/management/client/rest/dns.go b/management/client/rest/dns.go index ef9923b1f..0e2d15842 100644 --- a/management/client/rest/dns.go +++ b/management/client/rest/dns.go @@ -20,7 +20,9 @@ func (a *DNSAPI) ListNameserverGroups(ctx context.Context) ([]api.NameserverGrou if err != nil { return nil, err } - defer resp.Body.Close() + if resp.Body != nil { + defer resp.Body.Close() + } ret, err := parseResponse[[]api.NameserverGroup](resp) return ret, err } @@ -32,7 +34,9 @@ func (a *DNSAPI) GetNameserverGroup(ctx context.Context, nameserverGroupID strin if err != nil { return nil, err } - defer resp.Body.Close() + if resp.Body != nil { + defer resp.Body.Close() + } ret, err := parseResponse[api.NameserverGroup](resp) return &ret, err } @@ -48,7 +52,9 @@ func (a *DNSAPI) CreateNameserverGroup(ctx context.Context, request api.PostApiD if err != nil { return nil, err } - defer resp.Body.Close() + if resp.Body != nil { + defer resp.Body.Close() + } ret, err := parseResponse[api.NameserverGroup](resp) return &ret, err } @@ -64,7 +70,9 @@ func (a *DNSAPI) UpdateNameserverGroup(ctx context.Context, nameserverGroupID st if err != nil { return nil, err } - defer resp.Body.Close() + if resp.Body != nil { + defer resp.Body.Close() + } ret, err := parseResponse[api.NameserverGroup](resp) return &ret, err } @@ -76,7 +84,9 @@ func (a *DNSAPI) DeleteNameserverGroup(ctx context.Context, nameserverGroupID st if err != nil { return err } - defer resp.Body.Close() + if resp.Body != nil { + defer resp.Body.Close() + } return nil } @@ -88,7 +98,9 @@ func (a *DNSAPI) GetSettings(ctx context.Context) (*api.DNSSettings, error) { if err != nil { return nil, err } - defer resp.Body.Close() + if resp.Body != nil { + defer resp.Body.Close() + } ret, err := parseResponse[api.DNSSettings](resp) return &ret, err } @@ -104,7 +116,9 @@ func (a *DNSAPI) UpdateSettings(ctx context.Context, request api.PutApiDnsSettin if err != nil { return nil, err } - defer resp.Body.Close() + if resp.Body != nil { + defer resp.Body.Close() + } ret, err := parseResponse[api.DNSSettings](resp) return &ret, err } diff --git a/management/client/rest/events.go b/management/client/rest/events.go index 1157700ff..ed74fae39 100644 --- a/management/client/rest/events.go +++ b/management/client/rest/events.go @@ -18,7 +18,9 @@ func (a *EventsAPI) List(ctx context.Context) ([]api.Event, error) { if err != nil { return nil, err } - defer resp.Body.Close() + if resp.Body != nil { + defer resp.Body.Close() + } ret, err := parseResponse[[]api.Event](resp) return ret, err } diff --git a/management/client/rest/geo.go b/management/client/rest/geo.go index ed9090fe2..0bdcc0a22 100644 --- a/management/client/rest/geo.go +++ b/management/client/rest/geo.go @@ -18,7 +18,9 @@ func (a *GeoLocationAPI) ListCountries(ctx context.Context) ([]api.Country, erro if err != nil { return nil, err } - defer resp.Body.Close() + if resp.Body != nil { + defer resp.Body.Close() + } ret, err := parseResponse[[]api.Country](resp) return ret, err } @@ -30,7 +32,9 @@ func (a *GeoLocationAPI) ListCountryCities(ctx context.Context, countryCode stri if err != nil { return nil, err } - defer resp.Body.Close() + if resp.Body != nil { + defer resp.Body.Close() + } ret, err := parseResponse[[]api.City](resp) return ret, err } diff --git a/management/client/rest/groups.go b/management/client/rest/groups.go index feb664273..aac453b93 100644 --- a/management/client/rest/groups.go +++ b/management/client/rest/groups.go @@ -20,7 +20,9 @@ func (a *GroupsAPI) List(ctx context.Context) ([]api.Group, error) { if err != nil { return nil, err } - defer resp.Body.Close() + if resp.Body != nil { + defer resp.Body.Close() + } ret, err := parseResponse[[]api.Group](resp) return ret, err } @@ -32,7 +34,9 @@ func (a *GroupsAPI) Get(ctx context.Context, groupID string) (*api.Group, error) if err != nil { return nil, err } - defer resp.Body.Close() + if resp.Body != nil { + defer resp.Body.Close() + } ret, err := parseResponse[api.Group](resp) return &ret, err } @@ -48,7 +52,9 @@ func (a *GroupsAPI) Create(ctx context.Context, request api.PostApiGroupsJSONReq if err != nil { return nil, err } - defer resp.Body.Close() + if resp.Body != nil { + defer resp.Body.Close() + } ret, err := parseResponse[api.Group](resp) return &ret, err } @@ -64,7 +70,9 @@ func (a *GroupsAPI) Update(ctx context.Context, groupID string, request api.PutA if err != nil { return nil, err } - defer resp.Body.Close() + if resp.Body != nil { + defer resp.Body.Close() + } ret, err := parseResponse[api.Group](resp) return &ret, err } @@ -76,7 +84,9 @@ func (a *GroupsAPI) Delete(ctx context.Context, groupID string) error { if err != nil { return err } - defer resp.Body.Close() + if resp.Body != nil { + defer resp.Body.Close() + } return nil } diff --git a/management/client/rest/networks.go b/management/client/rest/networks.go index 2cdd6d73d..b211312c9 100644 --- a/management/client/rest/networks.go +++ b/management/client/rest/networks.go @@ -20,7 +20,9 @@ func (a *NetworksAPI) List(ctx context.Context) ([]api.Network, error) { if err != nil { return nil, err } - defer resp.Body.Close() + if resp.Body != nil { + defer resp.Body.Close() + } ret, err := parseResponse[[]api.Network](resp) return ret, err } @@ -32,7 +34,9 @@ func (a *NetworksAPI) Get(ctx context.Context, networkID string) (*api.Network, if err != nil { return nil, err } - defer resp.Body.Close() + if resp.Body != nil { + defer resp.Body.Close() + } ret, err := parseResponse[api.Network](resp) return &ret, err } @@ -48,7 +52,9 @@ func (a *NetworksAPI) Create(ctx context.Context, request api.PostApiNetworksJSO if err != nil { return nil, err } - defer resp.Body.Close() + if resp.Body != nil { + defer resp.Body.Close() + } ret, err := parseResponse[api.Network](resp) return &ret, err } @@ -64,7 +70,9 @@ func (a *NetworksAPI) Update(ctx context.Context, networkID string, request api. if err != nil { return nil, err } - defer resp.Body.Close() + if resp.Body != nil { + defer resp.Body.Close() + } ret, err := parseResponse[api.Network](resp) return &ret, err } @@ -76,7 +84,9 @@ func (a *NetworksAPI) Delete(ctx context.Context, networkID string) error { if err != nil { return err } - defer resp.Body.Close() + if resp.Body != nil { + defer resp.Body.Close() + } return nil } @@ -102,7 +112,9 @@ func (a *NetworkResourcesAPI) List(ctx context.Context) ([]api.NetworkResource, if err != nil { return nil, err } - defer resp.Body.Close() + if resp.Body != nil { + defer resp.Body.Close() + } ret, err := parseResponse[[]api.NetworkResource](resp) return ret, err } @@ -114,7 +126,9 @@ func (a *NetworkResourcesAPI) Get(ctx context.Context, networkResourceID string) if err != nil { return nil, err } - defer resp.Body.Close() + if resp.Body != nil { + defer resp.Body.Close() + } ret, err := parseResponse[api.NetworkResource](resp) return &ret, err } @@ -130,7 +144,9 @@ func (a *NetworkResourcesAPI) Create(ctx context.Context, request api.PostApiNet if err != nil { return nil, err } - defer resp.Body.Close() + if resp.Body != nil { + defer resp.Body.Close() + } ret, err := parseResponse[api.NetworkResource](resp) return &ret, err } @@ -146,7 +162,9 @@ func (a *NetworkResourcesAPI) Update(ctx context.Context, networkResourceID stri if err != nil { return nil, err } - defer resp.Body.Close() + if resp.Body != nil { + defer resp.Body.Close() + } ret, err := parseResponse[api.NetworkResource](resp) return &ret, err } @@ -158,7 +176,9 @@ func (a *NetworkResourcesAPI) Delete(ctx context.Context, networkResourceID stri if err != nil { return err } - defer resp.Body.Close() + if resp.Body != nil { + defer resp.Body.Close() + } return nil } @@ -184,7 +204,9 @@ func (a *NetworkRoutersAPI) List(ctx context.Context) ([]api.NetworkRouter, erro if err != nil { return nil, err } - defer resp.Body.Close() + if resp.Body != nil { + defer resp.Body.Close() + } ret, err := parseResponse[[]api.NetworkRouter](resp) return ret, err } @@ -196,7 +218,9 @@ func (a *NetworkRoutersAPI) Get(ctx context.Context, networkRouterID string) (*a if err != nil { return nil, err } - defer resp.Body.Close() + if resp.Body != nil { + defer resp.Body.Close() + } ret, err := parseResponse[api.NetworkRouter](resp) return &ret, err } @@ -212,7 +236,9 @@ func (a *NetworkRoutersAPI) Create(ctx context.Context, request api.PostApiNetwo if err != nil { return nil, err } - defer resp.Body.Close() + if resp.Body != nil { + defer resp.Body.Close() + } ret, err := parseResponse[api.NetworkRouter](resp) return &ret, err } @@ -228,7 +254,9 @@ func (a *NetworkRoutersAPI) Update(ctx context.Context, networkRouterID string, if err != nil { return nil, err } - defer resp.Body.Close() + if resp.Body != nil { + defer resp.Body.Close() + } ret, err := parseResponse[api.NetworkRouter](resp) return &ret, err } @@ -240,7 +268,9 @@ func (a *NetworkRoutersAPI) Delete(ctx context.Context, networkRouterID string) if err != nil { return err } - defer resp.Body.Close() + if resp.Body != nil { + defer resp.Body.Close() + } return nil } diff --git a/management/client/rest/peers.go b/management/client/rest/peers.go index 9d35f013c..2b1a65b4c 100644 --- a/management/client/rest/peers.go +++ b/management/client/rest/peers.go @@ -20,7 +20,9 @@ func (a *PeersAPI) List(ctx context.Context) ([]api.Peer, error) { if err != nil { return nil, err } - defer resp.Body.Close() + if resp.Body != nil { + defer resp.Body.Close() + } ret, err := parseResponse[[]api.Peer](resp) return ret, err } @@ -32,7 +34,9 @@ func (a *PeersAPI) Get(ctx context.Context, peerID string) (*api.Peer, error) { if err != nil { return nil, err } - defer resp.Body.Close() + if resp.Body != nil { + defer resp.Body.Close() + } ret, err := parseResponse[api.Peer](resp) return &ret, err } @@ -48,7 +52,9 @@ func (a *PeersAPI) Update(ctx context.Context, peerID string, request api.PutApi if err != nil { return nil, err } - defer resp.Body.Close() + if resp.Body != nil { + defer resp.Body.Close() + } ret, err := parseResponse[api.Peer](resp) return &ret, err } @@ -60,7 +66,9 @@ func (a *PeersAPI) Delete(ctx context.Context, peerID string) error { if err != nil { return err } - defer resp.Body.Close() + if resp.Body != nil { + defer resp.Body.Close() + } return nil } @@ -72,7 +80,9 @@ func (a *PeersAPI) ListAccessiblePeers(ctx context.Context, peerID string) ([]ap if err != nil { return nil, err } - defer resp.Body.Close() + if resp.Body != nil { + defer resp.Body.Close() + } ret, err := parseResponse[[]api.Peer](resp) return ret, err } diff --git a/management/client/rest/policies.go b/management/client/rest/policies.go index be6abafaf..975a95440 100644 --- a/management/client/rest/policies.go +++ b/management/client/rest/policies.go @@ -20,7 +20,9 @@ func (a *PoliciesAPI) List(ctx context.Context) ([]api.Policy, error) { if err != nil { return nil, err } - defer resp.Body.Close() + if resp.Body != nil { + defer resp.Body.Close() + } ret, err := parseResponse[[]api.Policy](resp) return ret, err } @@ -32,7 +34,9 @@ func (a *PoliciesAPI) Get(ctx context.Context, policyID string) (*api.Policy, er if err != nil { return nil, err } - defer resp.Body.Close() + if resp.Body != nil { + defer resp.Body.Close() + } ret, err := parseResponse[api.Policy](resp) return &ret, err } @@ -48,7 +52,9 @@ func (a *PoliciesAPI) Create(ctx context.Context, request api.PostApiPoliciesJSO if err != nil { return nil, err } - defer resp.Body.Close() + if resp.Body != nil { + defer resp.Body.Close() + } ret, err := parseResponse[api.Policy](resp) return &ret, err } @@ -64,7 +70,9 @@ func (a *PoliciesAPI) Update(ctx context.Context, policyID string, request api.P if err != nil { return nil, err } - defer resp.Body.Close() + if resp.Body != nil { + defer resp.Body.Close() + } ret, err := parseResponse[api.Policy](resp) return &ret, err } @@ -76,7 +84,9 @@ func (a *PoliciesAPI) Delete(ctx context.Context, policyID string) error { if err != nil { return err } - defer resp.Body.Close() + if resp.Body != nil { + defer resp.Body.Close() + } return nil } diff --git a/management/client/rest/posturechecks.go b/management/client/rest/posturechecks.go index 950d17ba0..7343957a5 100644 --- a/management/client/rest/posturechecks.go +++ b/management/client/rest/posturechecks.go @@ -20,7 +20,9 @@ func (a *PostureChecksAPI) List(ctx context.Context) ([]api.PostureCheck, error) if err != nil { return nil, err } - defer resp.Body.Close() + if resp.Body != nil { + defer resp.Body.Close() + } ret, err := parseResponse[[]api.PostureCheck](resp) return ret, err } @@ -32,7 +34,9 @@ func (a *PostureChecksAPI) Get(ctx context.Context, postureCheckID string) (*api if err != nil { return nil, err } - defer resp.Body.Close() + if resp.Body != nil { + defer resp.Body.Close() + } ret, err := parseResponse[api.PostureCheck](resp) return &ret, err } @@ -48,7 +52,9 @@ func (a *PostureChecksAPI) Create(ctx context.Context, request api.PostApiPostur if err != nil { return nil, err } - defer resp.Body.Close() + if resp.Body != nil { + defer resp.Body.Close() + } ret, err := parseResponse[api.PostureCheck](resp) return &ret, err } @@ -64,7 +70,9 @@ func (a *PostureChecksAPI) Update(ctx context.Context, postureCheckID string, re if err != nil { return nil, err } - defer resp.Body.Close() + if resp.Body != nil { + defer resp.Body.Close() + } ret, err := parseResponse[api.PostureCheck](resp) return &ret, err } @@ -76,7 +84,9 @@ func (a *PostureChecksAPI) Delete(ctx context.Context, postureCheckID string) er if err != nil { return err } - defer resp.Body.Close() + if resp.Body != nil { + defer resp.Body.Close() + } return nil } diff --git a/management/client/rest/routes.go b/management/client/rest/routes.go index bccbb8847..6ca4be2c5 100644 --- a/management/client/rest/routes.go +++ b/management/client/rest/routes.go @@ -20,7 +20,9 @@ func (a *RoutesAPI) List(ctx context.Context) ([]api.Route, error) { if err != nil { return nil, err } - defer resp.Body.Close() + if resp.Body != nil { + defer resp.Body.Close() + } ret, err := parseResponse[[]api.Route](resp) return ret, err } @@ -32,7 +34,9 @@ func (a *RoutesAPI) Get(ctx context.Context, routeID string) (*api.Route, error) if err != nil { return nil, err } - defer resp.Body.Close() + if resp.Body != nil { + defer resp.Body.Close() + } ret, err := parseResponse[api.Route](resp) return &ret, err } @@ -48,7 +52,9 @@ func (a *RoutesAPI) Create(ctx context.Context, request api.PostApiRoutesJSONReq if err != nil { return nil, err } - defer resp.Body.Close() + if resp.Body != nil { + defer resp.Body.Close() + } ret, err := parseResponse[api.Route](resp) return &ret, err } @@ -64,7 +70,9 @@ func (a *RoutesAPI) Update(ctx context.Context, routeID string, request api.PutA if err != nil { return nil, err } - defer resp.Body.Close() + if resp.Body != nil { + defer resp.Body.Close() + } ret, err := parseResponse[api.Route](resp) return &ret, err } @@ -76,7 +84,9 @@ func (a *RoutesAPI) Delete(ctx context.Context, routeID string) error { if err != nil { return err } - defer resp.Body.Close() + if resp.Body != nil { + defer resp.Body.Close() + } return nil } diff --git a/management/client/rest/setupkeys.go b/management/client/rest/setupkeys.go index 645614fcf..91f370663 100644 --- a/management/client/rest/setupkeys.go +++ b/management/client/rest/setupkeys.go @@ -20,7 +20,9 @@ func (a *SetupKeysAPI) List(ctx context.Context) ([]api.SetupKey, error) { if err != nil { return nil, err } - defer resp.Body.Close() + if resp.Body != nil { + defer resp.Body.Close() + } ret, err := parseResponse[[]api.SetupKey](resp) return ret, err } @@ -32,7 +34,9 @@ func (a *SetupKeysAPI) Get(ctx context.Context, setupKeyID string) (*api.SetupKe if err != nil { return nil, err } - defer resp.Body.Close() + if resp.Body != nil { + defer resp.Body.Close() + } ret, err := parseResponse[api.SetupKey](resp) return &ret, err } @@ -48,7 +52,9 @@ func (a *SetupKeysAPI) Create(ctx context.Context, request api.PostApiSetupKeysJ if err != nil { return nil, err } - defer resp.Body.Close() + if resp.Body != nil { + defer resp.Body.Close() + } ret, err := parseResponse[api.SetupKeyClear](resp) return &ret, err } @@ -64,7 +70,9 @@ func (a *SetupKeysAPI) Update(ctx context.Context, setupKeyID string, request ap if err != nil { return nil, err } - defer resp.Body.Close() + if resp.Body != nil { + defer resp.Body.Close() + } ret, err := parseResponse[api.SetupKey](resp) return &ret, err } @@ -76,7 +84,9 @@ func (a *SetupKeysAPI) Delete(ctx context.Context, setupKeyID string) error { if err != nil { return err } - defer resp.Body.Close() + if resp.Body != nil { + defer resp.Body.Close() + } return nil } diff --git a/management/client/rest/tokens.go b/management/client/rest/tokens.go index 3275bea81..7e5004147 100644 --- a/management/client/rest/tokens.go +++ b/management/client/rest/tokens.go @@ -20,7 +20,9 @@ func (a *TokensAPI) List(ctx context.Context, userID string) ([]api.PersonalAcce if err != nil { return nil, err } - defer resp.Body.Close() + if resp.Body != nil { + defer resp.Body.Close() + } ret, err := parseResponse[[]api.PersonalAccessToken](resp) return ret, err } @@ -32,7 +34,9 @@ func (a *TokensAPI) Get(ctx context.Context, userID, tokenID string) (*api.Perso if err != nil { return nil, err } - defer resp.Body.Close() + if resp.Body != nil { + defer resp.Body.Close() + } ret, err := parseResponse[api.PersonalAccessToken](resp) return &ret, err } @@ -48,7 +52,9 @@ func (a *TokensAPI) Create(ctx context.Context, userID string, request api.PostA if err != nil { return nil, err } - defer resp.Body.Close() + if resp.Body != nil { + defer resp.Body.Close() + } ret, err := parseResponse[api.PersonalAccessTokenGenerated](resp) return &ret, err } @@ -60,7 +66,9 @@ func (a *TokensAPI) Delete(ctx context.Context, userID, tokenID string) error { if err != nil { return err } - defer resp.Body.Close() + if resp.Body != nil { + defer resp.Body.Close() + } return nil } diff --git a/management/client/rest/users.go b/management/client/rest/users.go index 31ffad051..bb81796c0 100644 --- a/management/client/rest/users.go +++ b/management/client/rest/users.go @@ -20,7 +20,9 @@ func (a *UsersAPI) List(ctx context.Context) ([]api.User, error) { if err != nil { return nil, err } - defer resp.Body.Close() + if resp.Body != nil { + defer resp.Body.Close() + } ret, err := parseResponse[[]api.User](resp) return ret, err } @@ -36,7 +38,9 @@ func (a *UsersAPI) Create(ctx context.Context, request api.PostApiUsersJSONReque if err != nil { return nil, err } - defer resp.Body.Close() + if resp.Body != nil { + defer resp.Body.Close() + } ret, err := parseResponse[api.User](resp) return &ret, err } @@ -52,7 +56,9 @@ func (a *UsersAPI) Update(ctx context.Context, userID string, request api.PutApi if err != nil { return nil, err } - defer resp.Body.Close() + if resp.Body != nil { + defer resp.Body.Close() + } ret, err := parseResponse[api.User](resp) return &ret, err } @@ -64,7 +70,9 @@ func (a *UsersAPI) Delete(ctx context.Context, userID string) error { if err != nil { return err } - defer resp.Body.Close() + if resp.Body != nil { + defer resp.Body.Close() + } return nil } @@ -76,7 +84,9 @@ func (a *UsersAPI) ResendInvitation(ctx context.Context, userID string) error { if err != nil { return err } - defer resp.Body.Close() + if resp.Body != nil { + defer resp.Body.Close() + } return nil } @@ -88,7 +98,9 @@ func (a *UsersAPI) Current(ctx context.Context) (*api.User, error) { if err != nil { return nil, err } - defer resp.Body.Close() + if resp.Body != nil { + defer resp.Body.Close() + } ret, err := parseResponse[api.User](resp) return &ret, err diff --git a/management/client/rest/users_test.go b/management/client/rest/users_test.go index f68c5f083..715eb1661 100644 --- a/management/client/rest/users_test.go +++ b/management/client/rest/users_test.go @@ -30,11 +30,8 @@ var ( Issued: ptr("api"), LastLogin: &time.Time{}, Name: "M. Essam", - Permissions: &api.UserPermissions{ - DashboardView: ptr(api.UserPermissionsDashboardViewFull), - }, - Role: "user", - Status: api.UserStatusActive, + Role: "user", + Status: api.UserStatusActive, } ) diff --git a/management/server/account.go b/management/server/account.go index ab1ffe8b3..5c474a343 100644 --- a/management/server/account.go +++ b/management/server/account.go @@ -603,11 +603,15 @@ func (am *DefaultAccountManager) DeleteAccount(ctx context.Context, accountID, u } for _, otherUser := range account.Users { - if otherUser.IsServiceUser { + if otherUser.Id == userID { continue } - if otherUser.Id == userID { + if otherUser.IsServiceUser { + err = am.deleteServiceUser(ctx, accountID, userID, otherUser) + if err != nil { + return err + } continue } @@ -712,7 +716,7 @@ func (am *DefaultAccountManager) loadAccount(ctx context.Context, accountID any) log.WithContext(ctx).Debugf("account %s not found in cache, reloading", accountID) accountIDString := fmt.Sprintf("%v", accountID) - account, err := am.Store.GetAccount(ctx, accountIDString) + accountUsers, err := am.Store.GetAccountUsers(ctx, store.LockingStrengthShare, accountIDString) if err != nil { return nil, nil, err } @@ -721,7 +725,7 @@ func (am *DefaultAccountManager) loadAccount(ctx context.Context, accountID any) if err != nil { return nil, nil, err } - log.WithContext(ctx).Debugf("%d entries received from IdP management", len(userData)) + log.WithContext(ctx).Debugf("%d entries received from IdP management for account %s", len(userData), accountIDString) dataMap := make(map[string]*idp.UserData, len(userData)) for _, datum := range userData { @@ -729,7 +733,7 @@ func (am *DefaultAccountManager) loadAccount(ctx context.Context, accountID any) } matchedUserData := make([]*idp.UserData, 0) - for _, user := range account.Users { + for _, user := range accountUsers { if user.IsServiceUser { continue } diff --git a/management/server/account/manager.go b/management/server/account/manager.go index aed83349f..9bc4f9605 100644 --- a/management/server/account/manager.go +++ b/management/server/account/manager.go @@ -16,6 +16,7 @@ import ( "github.com/netbirdio/netbird/management/server/posture" "github.com/netbirdio/netbird/management/server/store" "github.com/netbirdio/netbird/management/server/types" + "github.com/netbirdio/netbird/management/server/users" "github.com/netbirdio/netbird/route" ) @@ -115,5 +116,5 @@ type Manager interface { CreateAccountByPrivateDomain(ctx context.Context, initiatorId, domain string) (*types.Account, error) UpdateToPrimaryAccount(ctx context.Context, accountId string) (*types.Account, error) GetOwnerInfo(ctx context.Context, accountId string) (*types.UserInfo, error) - GetCurrentUserInfo(ctx context.Context, accountID, userID string) (*types.UserInfo, error) + GetCurrentUserInfo(ctx context.Context, userAuth nbcontext.UserAuth) (*users.UserInfoWithPermissions, error) } diff --git a/management/server/account_test.go b/management/server/account_test.go index fe082d9a0..c5583d226 100644 --- a/management/server/account_test.go +++ b/management/server/account_test.go @@ -853,6 +853,42 @@ func TestAccountManager_DeleteAccount(t *testing.T) { t.Fatal(err) } + account.Users["service-user-1"] = &types.User{ + Id: "service-user-1", + Role: types.UserRoleAdmin, + IsServiceUser: true, + Issued: types.UserIssuedAPI, + PATs: map[string]*types.PersonalAccessToken{ + "pat-1": { + ID: "pat-1", + UserID: "service-user-1", + Name: "service-user-1", + HashedToken: "hashedToken", + CreatedAt: time.Now(), + }, + }, + } + account.Users[userId] = &types.User{ + Id: "service-user-2", + Role: types.UserRoleUser, + IsServiceUser: true, + Issued: types.UserIssuedAPI, + PATs: map[string]*types.PersonalAccessToken{ + "pat-2": { + ID: "pat-2", + UserID: userId, + Name: userId, + HashedToken: "hashedToken", + CreatedAt: time.Now(), + }, + }, + } + + err = manager.Store.SaveAccount(context.Background(), account) + if err != nil { + t.Fatal(err) + } + err = manager.DeleteAccount(context.Background(), account.Id, userId) if err != nil { t.Fatal(err) @@ -862,6 +898,14 @@ func TestAccountManager_DeleteAccount(t *testing.T) { if err == nil { t.Fatal(fmt.Errorf("expected to get an error when trying to get deleted account, got %v", getAccount)) } + + pats, err := manager.Store.GetUserPATs(context.Background(), store.LockingStrengthShare, "service-user-1") + require.NoError(t, err) + assert.Len(t, pats, 0) + + pats, err = manager.Store.GetUserPATs(context.Background(), store.LockingStrengthShare, userId) + require.NoError(t, err) + assert.Len(t, pats, 0) } func BenchmarkTest_GetAccountWithclaims(b *testing.B) { diff --git a/management/server/http/api/openapi.yml b/management/server/http/api/openapi.yml index 1dba20426..fb51038a9 100644 --- a/management/server/http/api/openapi.yml +++ b/management/server/http/api/openapi.yml @@ -216,11 +216,25 @@ components: UserPermissions: type: object properties: - dashboard_view: - description: User's permission to view the dashboard - type: string - enum: [ "limited", "blocked", "full" ] - example: limited + is_restricted: + type: boolean + description: Indicates whether this User's Peers view is restricted + modules: + type: object + additionalProperties: + type: object + additionalProperties: + type: boolean + propertyNames: + type: string + description: The operation type + propertyNames: + type: string + description: The module name + example: {"networks": { "read": true, "create": false, "update": false, "delete": false}, "peers": { "read": false, "create": false, "update": false, "delete": false} } + required: + - modules + - is_restricted UserRequest: type: object properties: diff --git a/management/server/http/api/types.gen.go b/management/server/http/api/types.gen.go index 1d50a4354..c9dec0569 100644 --- a/management/server/http/api/types.gen.go +++ b/management/server/http/api/types.gen.go @@ -178,13 +178,6 @@ const ( UserStatusInvited UserStatus = "invited" ) -// Defines values for UserPermissionsDashboardView. -const ( - UserPermissionsDashboardViewBlocked UserPermissionsDashboardView = "blocked" - UserPermissionsDashboardViewFull UserPermissionsDashboardView = "full" - UserPermissionsDashboardViewLimited UserPermissionsDashboardView = "limited" -) - // Defines values for GetApiEventsNetworkTrafficParamsType. const ( GetApiEventsNetworkTrafficParamsTypeTYPEDROP GetApiEventsNetworkTrafficParamsType = "TYPE_DROP" @@ -1769,13 +1762,11 @@ type UserCreateRequest struct { // UserPermissions defines model for UserPermissions. type UserPermissions struct { - // DashboardView User's permission to view the dashboard - DashboardView *UserPermissionsDashboardView `json:"dashboard_view,omitempty"` + // IsRestricted Indicates whether this User's Peers view is restricted + IsRestricted bool `json:"is_restricted"` + Modules map[string]map[string]bool `json:"modules"` } -// UserPermissionsDashboardView User's permission to view the dashboard -type UserPermissionsDashboardView string - // UserRequest defines model for UserRequest. type UserRequest struct { // AutoGroups Group IDs to auto-assign to peers registered by this user diff --git a/management/server/http/handlers/users/users_handler.go b/management/server/http/handlers/users/users_handler.go index c69c6b944..ac04b8e35 100644 --- a/management/server/http/handlers/users/users_handler.go +++ b/management/server/http/handlers/users/users_handler.go @@ -13,6 +13,7 @@ import ( "github.com/netbirdio/netbird/management/server/http/util" "github.com/netbirdio/netbird/management/server/status" "github.com/netbirdio/netbird/management/server/types" + "github.com/netbirdio/netbird/management/server/users" nbcontext "github.com/netbirdio/netbird/management/server/context" ) @@ -272,15 +273,33 @@ func (h *handler) getCurrentUser(w http.ResponseWriter, r *http.Request) { return } - accountID, userID := userAuth.AccountId, userAuth.UserId - - user, err := h.accountManager.GetCurrentUserInfo(ctx, accountID, userID) + user, err := h.accountManager.GetCurrentUserInfo(ctx, userAuth) if err != nil { util.WriteError(r.Context(), err, w) return } - util.WriteJSONObject(r.Context(), w, toUserResponse(user, userID)) + util.WriteJSONObject(r.Context(), w, toUserWithPermissionsResponse(user, userAuth.UserId)) +} + +func toUserWithPermissionsResponse(user *users.UserInfoWithPermissions, userID string) *api.User { + response := toUserResponse(user.UserInfo, userID) + + // stringify modules and operations keys + modules := make(map[string]map[string]bool) + for module, operations := range user.Permissions { + modules[string(module)] = make(map[string]bool) + for op, val := range operations { + modules[string(module)][string(op)] = val + } + } + + response.Permissions = &api.UserPermissions{ + IsRestricted: user.Restricted, + Modules: modules, + } + + return response } func toUserResponse(user *types.UserInfo, currenUserID string) *api.User { @@ -316,8 +335,5 @@ func toUserResponse(user *types.UserInfo, currenUserID string) *api.User { IsBlocked: user.IsBlocked, LastLogin: &user.LastLogin, Issued: &user.Issued, - Permissions: &api.UserPermissions{ - DashboardView: (*api.UserPermissionsDashboardView)(&user.Permissions.DashboardView), - }, } } diff --git a/management/server/http/handlers/users/users_handler_test.go b/management/server/http/handlers/users/users_handler_test.go index 604954819..58e33a6d5 100644 --- a/management/server/http/handlers/users/users_handler_test.go +++ b/management/server/http/handlers/users/users_handler_test.go @@ -13,12 +13,16 @@ import ( "github.com/gorilla/mux" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" nbcontext "github.com/netbirdio/netbird/management/server/context" "github.com/netbirdio/netbird/management/server/http/api" "github.com/netbirdio/netbird/management/server/mock_server" + "github.com/netbirdio/netbird/management/server/permissions/modules" + "github.com/netbirdio/netbird/management/server/permissions/roles" "github.com/netbirdio/netbird/management/server/status" "github.com/netbirdio/netbird/management/server/types" + "github.com/netbirdio/netbird/management/server/users" ) const ( @@ -107,7 +111,7 @@ func initUsersTestData() *handler { return nil, status.Errorf(status.NotFound, "user with ID %s does not exists", userID) } - info, err := update.Copy().ToUserInfo(nil, &types.Settings{RegularUsersViewBlocked: false}) + info, err := update.Copy().ToUserInfo(nil) if err != nil { return nil, err } @@ -124,8 +128,8 @@ func initUsersTestData() *handler { return nil }, - GetCurrentUserInfoFunc: func(ctx context.Context, accountID, userID string) (*types.UserInfo, error) { - switch userID { + GetCurrentUserInfoFunc: func(ctx context.Context, userAuth nbcontext.UserAuth) (*users.UserInfoWithPermissions, error) { + switch userAuth.UserId { case "not-found": return nil, status.NewUserNotFoundError("not-found") case "not-of-account": @@ -135,52 +139,68 @@ func initUsersTestData() *handler { case "service-user": return nil, status.NewPermissionDeniedError() case "owner": - return &types.UserInfo{ - ID: "owner", - Name: "", - Role: "owner", - Status: "active", - IsServiceUser: false, - IsBlocked: false, - NonDeletable: false, - Issued: "api", - Permissions: types.UserPermissions{ - DashboardView: "full", + return &users.UserInfoWithPermissions{ + UserInfo: &types.UserInfo{ + ID: "owner", + Name: "", + Role: "owner", + Status: "active", + IsServiceUser: false, + IsBlocked: false, + NonDeletable: false, + Issued: "api", }, + Permissions: mergeRolePermissions(roles.Owner), }, nil case "regular-user": - return &types.UserInfo{ - ID: "regular-user", - Name: "", - Role: "user", - Status: "active", - IsServiceUser: false, - IsBlocked: false, - NonDeletable: false, - Issued: "api", - Permissions: types.UserPermissions{ - DashboardView: "limited", + return &users.UserInfoWithPermissions{ + UserInfo: &types.UserInfo{ + ID: "regular-user", + Name: "", + Role: "user", + Status: "active", + IsServiceUser: false, + IsBlocked: false, + NonDeletable: false, + Issued: "api", }, + Permissions: mergeRolePermissions(roles.User), }, nil case "admin-user": - return &types.UserInfo{ - ID: "admin-user", - Name: "", - Role: "admin", - Status: "active", - IsServiceUser: false, - IsBlocked: false, - NonDeletable: false, - LastLogin: time.Time{}, - Issued: "api", - Permissions: types.UserPermissions{ - DashboardView: "full", + return &users.UserInfoWithPermissions{ + UserInfo: &types.UserInfo{ + ID: "admin-user", + Name: "", + Role: "admin", + Status: "active", + IsServiceUser: false, + IsBlocked: false, + NonDeletable: false, + LastLogin: time.Time{}, + Issued: "api", }, + Permissions: mergeRolePermissions(roles.Admin), + }, nil + case "restricted-user": + return &users.UserInfoWithPermissions{ + UserInfo: &types.UserInfo{ + ID: "restricted-user", + Name: "", + Role: "user", + Status: "active", + IsServiceUser: false, + IsBlocked: false, + NonDeletable: false, + LastLogin: time.Time{}, + Issued: "api", + }, + Permissions: mergeRolePermissions(roles.User), + Restricted: true, }, nil } - return nil, fmt.Errorf("user id %s not handled", userID) + return nil, fmt.Errorf("user id %s not handled", userAuth.UserId) }, }, } @@ -546,6 +566,7 @@ func TestCurrentUser(t *testing.T) { name string expectedStatus int requestAuth nbcontext.UserAuth + expectedResult *api.User }{ { name: "without auth", @@ -575,16 +596,78 @@ func TestCurrentUser(t *testing.T) { name: "owner", requestAuth: nbcontext.UserAuth{UserId: "owner"}, expectedStatus: http.StatusOK, + expectedResult: &api.User{ + Id: "owner", + Role: "owner", + Status: "active", + IsBlocked: false, + IsCurrent: ptr(true), + IsServiceUser: ptr(false), + AutoGroups: []string{}, + Issued: ptr("api"), + LastLogin: ptr(time.Time{}), + Permissions: &api.UserPermissions{ + Modules: stringifyPermissionsKeys(mergeRolePermissions(roles.Owner)), + }, + }, }, { name: "regular user", requestAuth: nbcontext.UserAuth{UserId: "regular-user"}, expectedStatus: http.StatusOK, + expectedResult: &api.User{ + Id: "regular-user", + Role: "user", + Status: "active", + IsBlocked: false, + IsCurrent: ptr(true), + IsServiceUser: ptr(false), + AutoGroups: []string{}, + Issued: ptr("api"), + LastLogin: ptr(time.Time{}), + Permissions: &api.UserPermissions{ + Modules: stringifyPermissionsKeys(mergeRolePermissions(roles.User)), + }, + }, }, { name: "admin user", requestAuth: nbcontext.UserAuth{UserId: "admin-user"}, expectedStatus: http.StatusOK, + expectedResult: &api.User{ + Id: "admin-user", + Role: "admin", + Status: "active", + IsBlocked: false, + IsCurrent: ptr(true), + IsServiceUser: ptr(false), + AutoGroups: []string{}, + Issued: ptr("api"), + LastLogin: ptr(time.Time{}), + Permissions: &api.UserPermissions{ + Modules: stringifyPermissionsKeys(mergeRolePermissions(roles.Admin)), + }, + }, + }, + { + name: "restricted user", + requestAuth: nbcontext.UserAuth{UserId: "restricted-user"}, + expectedStatus: http.StatusOK, + expectedResult: &api.User{ + Id: "restricted-user", + Role: "user", + Status: "active", + IsBlocked: false, + IsCurrent: ptr(true), + IsServiceUser: ptr(false), + AutoGroups: []string{}, + Issued: ptr("api"), + LastLogin: ptr(time.Time{}), + Permissions: &api.UserPermissions{ + IsRestricted: true, + Modules: stringifyPermissionsKeys(mergeRolePermissions(roles.User)), + }, + }, }, } @@ -603,10 +686,42 @@ func TestCurrentUser(t *testing.T) { res := rr.Result() defer res.Body.Close() - if status := rr.Code; status != tc.expectedStatus { - t.Fatalf("handler returned wrong status code: got %v want %v", - status, tc.expectedStatus) + assert.Equal(t, tc.expectedStatus, rr.Code, "handler returned wrong status code") + + if tc.expectedResult != nil { + var result api.User + require.NoError(t, json.NewDecoder(res.Body).Decode(&result)) + assert.EqualValues(t, *tc.expectedResult, result) } }) } } + +func ptr[T any, PT *T](x T) PT { + return &x +} + +func mergeRolePermissions(role roles.RolePermissions) roles.Permissions { + permissions := roles.Permissions{} + + for k := range modules.All { + if rolePermissions, ok := role.Permissions[k]; ok { + permissions[k] = rolePermissions + continue + } + permissions[k] = role.AutoAllowNew + } + + return permissions +} + +func stringifyPermissionsKeys(permissions roles.Permissions) map[string]map[string]bool { + modules := make(map[string]map[string]bool) + for module, operations := range permissions { + modules[string(module)] = make(map[string]bool) + for op, val := range operations { + modules[string(module)][string(op)] = val + } + } + return modules +} diff --git a/management/server/migration/migration.go b/management/server/migration/migration.go index d7abbad47..c8a852e0a 100644 --- a/management/server/migration/migration.go +++ b/management/server/migration/migration.go @@ -352,3 +352,24 @@ func MigrateNewField[T any](ctx context.Context, db *gorm.DB, columnName string, log.WithContext(ctx).Infof("Migration of empty %s to default value in table %s completed", columnName, tableName) return nil } + +func DropIndex[T any](ctx context.Context, db *gorm.DB, indexName string) error { + var model T + + if !db.Migrator().HasTable(&model) { + log.WithContext(ctx).Debugf("table for %T does not exist, no migration needed", model) + return nil + } + + if !db.Migrator().HasIndex(&model, indexName) { + log.WithContext(ctx).Debugf("index %s does not exist in table %T, no migration needed", indexName, model) + return nil + } + + if err := db.Migrator().DropIndex(&model, indexName); err != nil { + return fmt.Errorf("failed to drop index %s: %w", indexName, err) + } + + log.WithContext(ctx).Infof("dropped index %s from table %T", indexName, model) + return nil +} diff --git a/management/server/migration/migration_test.go b/management/server/migration/migration_test.go index e907d6853..94377930a 100644 --- a/management/server/migration/migration_test.go +++ b/management/server/migration/migration_test.go @@ -227,3 +227,25 @@ func TestMigrateSetupKeyToHashedSetupKey_ForAlreadyMigratedKey_Case2(t *testing. assert.Equal(t, "9+FQcmNd2GCxIK+SvHmtp6PPGV4MKEicDS+xuSQmvlE=", key.Key, "Key should be hashed") } + +func TestDropIndex(t *testing.T) { + db := setupDatabase(t) + + err := db.AutoMigrate(&types.SetupKey{}) + require.NoError(t, err, "Failed to auto-migrate tables") + + err = db.Save(&types.SetupKey{ + Id: "1", + Key: "9+FQcmNd2GCxIK+SvHmtp6PPGV4MKEicDS+xuSQmvlE=", + }).Error + require.NoError(t, err, "Failed to insert setup key") + + exist := db.Migrator().HasIndex(&types.SetupKey{}, "idx_setup_keys_account_id") + assert.True(t, exist, "Should have the index") + + err = migration.DropIndex[types.SetupKey](context.Background(), db, "idx_setup_keys_account_id") + require.NoError(t, err, "Migration should not fail to remove index") + + exist = db.Migrator().HasIndex(&types.SetupKey{}, "idx_setup_keys_account_id") + assert.False(t, exist, "Should not have the index") +} diff --git a/management/server/mock_server/account_mock.go b/management/server/mock_server/account_mock.go index 2b57e6888..0dd3f927e 100644 --- a/management/server/mock_server/account_mock.go +++ b/management/server/mock_server/account_mock.go @@ -19,6 +19,7 @@ import ( "github.com/netbirdio/netbird/management/server/posture" "github.com/netbirdio/netbird/management/server/store" "github.com/netbirdio/netbird/management/server/types" + "github.com/netbirdio/netbird/management/server/users" "github.com/netbirdio/netbird/route" ) @@ -115,7 +116,7 @@ type MockAccountManager struct { CreateAccountByPrivateDomainFunc func(ctx context.Context, initiatorId, domain string) (*types.Account, error) UpdateToPrimaryAccountFunc func(ctx context.Context, accountId string) (*types.Account, error) GetOwnerInfoFunc func(ctx context.Context, accountID string) (*types.UserInfo, error) - GetCurrentUserInfoFunc func(ctx context.Context, accountID, userID string) (*types.UserInfo, error) + GetCurrentUserInfoFunc func(ctx context.Context, userAuth nbcontext.UserAuth) (*users.UserInfoWithPermissions, error) GetAccountMetaFunc func(ctx context.Context, accountID, userID string) (*types.AccountMeta, error) } @@ -882,9 +883,9 @@ func (am *MockAccountManager) GetOwnerInfo(ctx context.Context, accountId string return nil, status.Errorf(codes.Unimplemented, "method GetOwnerInfo is not implemented") } -func (am *MockAccountManager) GetCurrentUserInfo(ctx context.Context, accountID, userID string) (*types.UserInfo, error) { +func (am *MockAccountManager) GetCurrentUserInfo(ctx context.Context, userAuth nbcontext.UserAuth) (*users.UserInfoWithPermissions, error) { if am.GetCurrentUserInfoFunc != nil { - return am.GetCurrentUserInfoFunc(ctx, accountID, userID) + return am.GetCurrentUserInfoFunc(ctx, userAuth) } return nil, status.Errorf(codes.Unimplemented, "method GetCurrentUserInfo is not implemented") } diff --git a/management/server/networks/resources/types/resource.go b/management/server/networks/resources/types/resource.go index ecac0a724..04c63608d 100644 --- a/management/server/networks/resources/types/resource.go +++ b/management/server/networks/resources/types/resource.go @@ -30,7 +30,7 @@ func (p NetworkResourceType) String() string { } type NetworkResource struct { - ID string `gorm:"index"` + ID string `gorm:"primaryKey"` NetworkID string `gorm:"index"` AccountID string `gorm:"index"` Name string diff --git a/management/server/networks/routers/types/router.go b/management/server/networks/routers/types/router.go index 5158ebb12..71465868f 100644 --- a/management/server/networks/routers/types/router.go +++ b/management/server/networks/routers/types/router.go @@ -10,7 +10,7 @@ import ( ) type NetworkRouter struct { - ID string `gorm:"index"` + ID string `gorm:"primaryKey"` NetworkID string `gorm:"index"` AccountID string `gorm:"index"` Peer string diff --git a/management/server/networks/types/network.go b/management/server/networks/types/network.go index a4ba7b821..d1c7f2b33 100644 --- a/management/server/networks/types/network.go +++ b/management/server/networks/types/network.go @@ -7,7 +7,7 @@ import ( ) type Network struct { - ID string `gorm:"index"` + ID string `gorm:"primaryKey"` AccountID string `gorm:"index"` Name string Description string diff --git a/management/server/peer.go b/management/server/peer.go index 908610fbe..9ff80442e 100644 --- a/management/server/peer.go +++ b/management/server/peer.go @@ -49,20 +49,9 @@ func (am *DefaultAccountManager) GetPeers(ctx context.Context, accountID, userID return nil, err } - peers := make([]*nbpeer.Peer, 0) - peersMap := make(map[string]*nbpeer.Peer) - - for _, peer := range accountPeers { - if user.IsRegularUser() && user.Id != peer.UserID { - // only display peers that belong to the current user if the current user is not an admin - continue - } - peers = append(peers, peer) - peersMap[peer.ID] = peer - } - + // @note if the user has permission to read peers it shows all account peers if allowed { - return peers, nil + return accountPeers, nil } settings, err := am.Store.GetAccountSettings(ctx, store.LockingStrengthShare, accountID) @@ -70,10 +59,22 @@ func (am *DefaultAccountManager) GetPeers(ctx context.Context, accountID, userID return nil, fmt.Errorf("failed to get account settings: %w", err) } - if settings.RegularUsersViewBlocked { + if user.IsRestrictable() && settings.RegularUsersViewBlocked { return []*nbpeer.Peer{}, nil } + // @note if it does not have permission read peers then only display it's own peers + peers := make([]*nbpeer.Peer, 0) + peersMap := make(map[string]*nbpeer.Peer) + + for _, peer := range accountPeers { + if user.Id != peer.UserID { + continue + } + peers = append(peers, peer) + peersMap[peer.ID] = peer + } + return am.getUserAccessiblePeers(ctx, accountID, peersMap, peers) } diff --git a/management/server/permissions/manager.go b/management/server/permissions/manager.go index 50a44eb0f..ebbce5d4a 100644 --- a/management/server/permissions/manager.go +++ b/management/server/permissions/manager.go @@ -20,6 +20,8 @@ type Manager interface { ValidateUserPermissions(ctx context.Context, accountID, userID string, module modules.Module, operation operations.Operation) (bool, error) ValidateRoleModuleAccess(ctx context.Context, accountID string, role roles.RolePermissions, module modules.Module, operation operations.Operation) bool ValidateAccountAccess(ctx context.Context, accountID string, user *types.User, allowOwnerAndAdmin bool) error + + GetPermissionsByRole(ctx context.Context, role types.UserRole) (roles.Permissions, error) } type managerImpl struct { @@ -96,3 +98,22 @@ func (m *managerImpl) ValidateAccountAccess(ctx context.Context, accountID strin } return nil } + +func (m *managerImpl) GetPermissionsByRole(ctx context.Context, role types.UserRole) (roles.Permissions, error) { + roleMap, ok := roles.RolesMap[role] + if !ok { + return roles.Permissions{}, status.NewUserRoleNotFoundError(string(role)) + } + + permissions := roles.Permissions{} + + for k := range modules.All { + if rolePermissions, ok := roleMap.Permissions[k]; ok { + permissions[k] = rolePermissions + continue + } + permissions[k] = roleMap.AutoAllowNew + } + + return permissions, nil +} diff --git a/management/server/permissions/manager_mock.go b/management/server/permissions/manager_mock.go index 266a24270..fa115d628 100644 --- a/management/server/permissions/manager_mock.go +++ b/management/server/permissions/manager_mock.go @@ -38,6 +38,21 @@ func (m *MockManager) EXPECT() *MockManagerMockRecorder { return m.recorder } +// GetPermissionsByRole mocks base method. +func (m *MockManager) GetPermissionsByRole(ctx context.Context, role types.UserRole) (roles.Permissions, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetPermissionsByRole", ctx, role) + ret0, _ := ret[0].(roles.Permissions) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// GetPermissionsByRole indicates an expected call of GetPermissionsByRole. +func (mr *MockManagerMockRecorder) GetPermissionsByRole(ctx, role interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetPermissionsByRole", reflect.TypeOf((*MockManager)(nil).GetPermissionsByRole), ctx, role) +} + // ValidateAccountAccess mocks base method. func (m *MockManager) ValidateAccountAccess(ctx context.Context, accountID string, user *types.User, allowOwnerAndAdmin bool) error { m.ctrl.T.Helper() diff --git a/management/server/permissions/modules/module.go b/management/server/permissions/modules/module.go index 4c42b6190..3d021a235 100644 --- a/management/server/permissions/modules/module.go +++ b/management/server/permissions/modules/module.go @@ -17,3 +17,19 @@ const ( SetupKeys Module = "setup_keys" Pats Module = "pats" ) + +var All = map[Module]struct{}{ + Networks: {}, + Peers: {}, + Groups: {}, + Settings: {}, + Accounts: {}, + Dns: {}, + Nameservers: {}, + Events: {}, + Policies: {}, + Routes: {}, + Users: {}, + SetupKeys: {}, + Pats: {}, +} diff --git a/management/server/permissions/roles/network_admin.go b/management/server/permissions/roles/network_admin.go index 761933386..e95d58381 100644 --- a/management/server/permissions/roles/network_admin.go +++ b/management/server/permissions/roles/network_admin.go @@ -23,9 +23,9 @@ var NetworkAdmin = RolePermissions{ }, modules.Groups: { operations.Read: true, - operations.Create: false, - operations.Update: false, - operations.Delete: false, + operations.Create: true, + operations.Update: true, + operations.Delete: true, }, modules.Settings: { operations.Read: true, @@ -87,5 +87,11 @@ var NetworkAdmin = RolePermissions{ operations.Update: true, operations.Delete: true, }, + modules.Peers: { + operations.Read: true, + operations.Create: false, + operations.Update: false, + operations.Delete: false, + }, }, } diff --git a/management/server/store/sql_store.go b/management/server/store/sql_store.go index 7d3b288e0..d568460f9 100644 --- a/management/server/store/sql_store.go +++ b/management/server/store/sql_store.go @@ -802,7 +802,7 @@ func (s *SqlStore) GetAccountByPeerPubKey(ctx context.Context, peerKey string) ( func (s *SqlStore) GetAnyAccountID(ctx context.Context) (string, error) { var account types.Account - result := s.db.WithContext(ctx).Select("id").Limit(1).Find(&account) + result := s.db.WithContext(ctx).Select("id").Order("created_at desc").Limit(1).Find(&account) if result.Error != nil { return "", status.NewGetAccountFromStoreError(result.Error) } @@ -1683,18 +1683,26 @@ func (s *SqlStore) SavePolicy(ctx context.Context, lockStrength LockingStrength, } func (s *SqlStore) DeletePolicy(ctx context.Context, lockStrength LockingStrength, accountID, policyID string) error { - result := s.db.Clauses(clause.Locking{Strength: string(lockStrength)}). - Delete(&types.Policy{}, accountAndIDQueryCondition, accountID, policyID) - if err := result.Error; err != nil { - log.WithContext(ctx).Errorf("failed to delete policy from store: %s", err) - return status.Errorf(status.Internal, "failed to delete policy from store") - } + return s.db.WithContext(ctx).Transaction(func(tx *gorm.DB) error { + if err := tx.Where("policy_id = ?", policyID).Delete(&types.PolicyRule{}).Error; err != nil { + return fmt.Errorf("delete policy rules: %w", err) + } - if result.RowsAffected == 0 { - return status.NewPolicyNotFoundError(policyID) - } + result := tx.Clauses(clause.Locking{Strength: string(lockStrength)}). + Where(accountAndIDQueryCondition, accountID, policyID). + Delete(&types.Policy{}) - return nil + if err := result.Error; err != nil { + log.WithContext(ctx).Errorf("failed to delete policy from store: %s", err) + return status.Errorf(status.Internal, "failed to delete policy from store") + } + + if result.RowsAffected == 0 { + return status.NewPolicyNotFoundError(policyID) + } + + return nil + }) } // GetAccountPostureChecks retrieves posture checks for an account. diff --git a/management/server/store/sql_store_test.go b/management/server/store/sql_store_test.go index 8bd8ce098..8e99b34e1 100644 --- a/management/server/store/sql_store_test.go +++ b/management/server/store/sql_store_test.go @@ -60,10 +60,10 @@ func Test_NewStore(t *testing.T) { runTestForAllEngines(t, "", func(t *testing.T, store Store) { if store == nil { - t.Errorf("expected to create a new Store") + t.Fatalf("expected to create a new Store") } if len(store.GetAllAccounts(context.Background())) != 0 { - t.Errorf("expected to create a new empty Accounts map when creating a new FileStore") + t.Fatalf("expected to create a new empty Accounts map when creating a new FileStore") } }) } @@ -1115,7 +1115,7 @@ func TestSqlite_CreateAndGetObjectInTransaction(t *testing.T) { group := &types.Group{ ID: "group-id", - AccountID: "account-id", + AccountID: "bf1c8084-ba50-4ce7-9439-34653001fc3b", Name: "group-name", Issued: "api", Peers: nil, diff --git a/management/server/store/store.go b/management/server/store/store.go index 73b366831..4699ecdf0 100644 --- a/management/server/store/store.go +++ b/management/server/store/store.go @@ -315,6 +315,15 @@ func getMigrations(ctx context.Context) []migrationFunc { func(db *gorm.DB) error { return migration.MigrateNewField[routerTypes.NetworkRouter](ctx, db, "enabled", true) }, + func(db *gorm.DB) error { + return migration.DropIndex[networkTypes.Network](ctx, db, "idx_networks_id") + }, + func(db *gorm.DB) error { + return migration.DropIndex[resourceTypes.NetworkResource](ctx, db, "idx_network_resources_id") + }, + func(db *gorm.DB) error { + return migration.DropIndex[routerTypes.NetworkRouter](ctx, db, "idx_network_routers_id") + }, } } diff --git a/management/server/types/group.go b/management/server/types/group.go index 00a28fa77..1b321387c 100644 --- a/management/server/types/group.go +++ b/management/server/types/group.go @@ -14,7 +14,7 @@ const ( // Group of the peers for ACL type Group struct { // ID of the group - ID string + ID string `gorm:"primaryKey"` // AccountID is a reference to Account that this object belongs AccountID string `json:"-" gorm:"index"` diff --git a/management/server/types/user.go b/management/server/types/user.go index a2596b3cb..783fe14da 100644 --- a/management/server/types/user.go +++ b/management/server/types/user.go @@ -65,11 +65,6 @@ type UserInfo struct { LastLogin time.Time `json:"last_login"` Issued string `json:"issued"` IntegrationReference integration_reference.IntegrationReference `json:"-"` - Permissions UserPermissions `json:"permissions"` -} - -type UserPermissions struct { - DashboardView string `json:"dashboard_view"` } // User represents a user of the system @@ -132,21 +127,18 @@ func (u *User) IsRegularUser() bool { return !u.HasAdminPower() && !u.IsServiceUser } +// IsRestrictable checks whether a user is in a restrictable role. +func (u *User) IsRestrictable() bool { + return u.Role == UserRoleUser || u.Role == UserRoleBillingAdmin +} + // ToUserInfo converts a User object to a UserInfo object. -func (u *User) ToUserInfo(userData *idp.UserData, settings *Settings) (*UserInfo, error) { +func (u *User) ToUserInfo(userData *idp.UserData) (*UserInfo, error) { autoGroups := u.AutoGroups if autoGroups == nil { autoGroups = []string{} } - dashboardViewPermissions := "full" - if !u.HasAdminPower() { - dashboardViewPermissions = "limited" - if settings.RegularUsersViewBlocked { - dashboardViewPermissions = "blocked" - } - } - if userData == nil { return &UserInfo{ ID: u.Id, @@ -159,9 +151,6 @@ func (u *User) ToUserInfo(userData *idp.UserData, settings *Settings) (*UserInfo IsBlocked: u.Blocked, LastLogin: u.GetLastLogin(), Issued: u.Issued, - Permissions: UserPermissions{ - DashboardView: dashboardViewPermissions, - }, }, nil } if userData.ID != u.Id { @@ -184,9 +173,6 @@ func (u *User) ToUserInfo(userData *idp.UserData, settings *Settings) (*UserInfo IsBlocked: u.Blocked, LastLogin: u.GetLastLogin(), Issued: u.Issued, - Permissions: UserPermissions{ - DashboardView: dashboardViewPermissions, - }, }, nil } diff --git a/management/server/user.go b/management/server/user.go index b46ed24cf..44ad3b68f 100644 --- a/management/server/user.go +++ b/management/server/user.go @@ -12,6 +12,7 @@ import ( "github.com/netbirdio/netbird/management/server/activity" nbContext "github.com/netbirdio/netbird/management/server/context" + nbcontext "github.com/netbirdio/netbird/management/server/context" "github.com/netbirdio/netbird/management/server/idp" nbpeer "github.com/netbirdio/netbird/management/server/peer" "github.com/netbirdio/netbird/management/server/permissions/modules" @@ -19,6 +20,7 @@ import ( "github.com/netbirdio/netbird/management/server/status" "github.com/netbirdio/netbird/management/server/store" "github.com/netbirdio/netbird/management/server/types" + "github.com/netbirdio/netbird/management/server/users" "github.com/netbirdio/netbird/management/server/util" ) @@ -122,11 +124,6 @@ func (am *DefaultAccountManager) inviteNewUser(ctx context.Context, accountID, u CreatedAt: time.Now().UTC(), } - settings, err := am.Store.GetAccountSettings(ctx, store.LockingStrengthShare, accountID) - if err != nil { - return nil, err - } - if err = am.Store.SaveUser(ctx, store.LockingStrengthUpdate, newUser); err != nil { return nil, err } @@ -138,7 +135,7 @@ func (am *DefaultAccountManager) inviteNewUser(ctx context.Context, accountID, u am.StoreEvent(ctx, userID, newUser.Id, accountID, activity.UserInvited, nil) - return newUser.ToUserInfo(idpUser, settings) + return newUser.ToUserInfo(idpUser) } // createNewIdpUser validates the invite and creates a new user in the IdP @@ -360,6 +357,7 @@ func (am *DefaultAccountManager) CreatePAT(ctx context.Context, accountID string return nil, err } + // @note this is essential to prevent non admin users with Pats create permission frpm creating one for a service user if initiatorUserID != targetUserID && !(initiatorUser.HasAdminPower() && targetUser.IsServiceUser) { return nil, status.NewAdminPermissionError() } @@ -727,19 +725,14 @@ func handleOwnerRoleTransfer(ctx context.Context, transaction store.Store, initi // If the AccountManager has a non-nil idpManager and the User is not a service user, // it will attempt to look up the UserData from the cache. func (am *DefaultAccountManager) getUserInfo(ctx context.Context, user *types.User, accountID string) (*types.UserInfo, error) { - settings, err := am.Store.GetAccountSettings(ctx, store.LockingStrengthShare, accountID) - if err != nil { - return nil, err - } - if !isNil(am.idpManager) && !user.IsServiceUser { userData, err := am.lookupUserInCache(ctx, user.Id, accountID) if err != nil { return nil, err } - return user.ToUserInfo(userData, settings) + return user.ToUserInfo(userData) } - return user.ToUserInfo(nil, settings) + return user.ToUserInfo(nil) } // validateUserUpdate validates the update operation for a user. @@ -879,17 +872,12 @@ func (am *DefaultAccountManager) BuildUserInfosForAccount(ctx context.Context, a queriedUsers = append(queriedUsers, usersFromIntegration...) } - settings, err := am.Store.GetAccountSettings(ctx, store.LockingStrengthShare, accountID) - if err != nil { - return nil, err - } - userInfosMap := make(map[string]*types.UserInfo) // in case of self-hosted, or IDP doesn't return anything, we will return the locally stored userInfo if len(queriedUsers) == 0 { for _, accountUser := range accountUsers { - info, err := accountUser.ToUserInfo(nil, settings) + info, err := accountUser.ToUserInfo(nil) if err != nil { return nil, err } @@ -902,7 +890,7 @@ func (am *DefaultAccountManager) BuildUserInfosForAccount(ctx context.Context, a for _, localUser := range accountUsers { var info *types.UserInfo if queriedUser, contains := findUserInIDPUserdata(localUser.Id, queriedUsers); contains { - info, err = localUser.ToUserInfo(queriedUser, settings) + info, err = localUser.ToUserInfo(queriedUser) if err != nil { return nil, err } @@ -912,14 +900,6 @@ func (am *DefaultAccountManager) BuildUserInfosForAccount(ctx context.Context, a name = localUser.ServiceUserName } - dashboardViewPermissions := "full" - if !localUser.HasAdminPower() { - dashboardViewPermissions = "limited" - if settings.RegularUsersViewBlocked { - dashboardViewPermissions = "blocked" - } - } - info = &types.UserInfo{ ID: localUser.Id, Email: "", @@ -929,7 +909,6 @@ func (am *DefaultAccountManager) BuildUserInfosForAccount(ctx context.Context, a Status: string(types.UserStatusActive), IsServiceUser: localUser.IsServiceUser, NonDeletable: localUser.NonDeletable, - Permissions: types.UserPermissions{DashboardView: dashboardViewPermissions}, } } userInfosMap[info.ID] = info @@ -1239,8 +1218,10 @@ func validateUserInvite(invite *types.UserInfo) error { return nil } -// GetCurrentUserInfo retrieves the account's current user info -func (am *DefaultAccountManager) GetCurrentUserInfo(ctx context.Context, accountID, userID string) (*types.UserInfo, error) { +// GetCurrentUserInfo retrieves the account's current user info and permissions +func (am *DefaultAccountManager) GetCurrentUserInfo(ctx context.Context, userAuth nbcontext.UserAuth) (*users.UserInfoWithPermissions, error) { + accountID, userID := userAuth.AccountId, userAuth.UserId + user, err := am.Store.GetUserByUserID(ctx, store.LockingStrengthShare, userID) if err != nil { return nil, err @@ -1258,10 +1239,25 @@ func (am *DefaultAccountManager) GetCurrentUserInfo(ctx context.Context, account return nil, err } + settings, err := am.Store.GetAccountSettings(ctx, store.LockingStrengthShare, accountID) + if err != nil { + return nil, err + } + userInfo, err := am.getUserInfo(ctx, user, accountID) if err != nil { return nil, err } - return userInfo, nil + userWithPermissions := &users.UserInfoWithPermissions{ + UserInfo: userInfo, + Restricted: !userAuth.IsChild && user.IsRestrictable() && settings.RegularUsersViewBlocked, + } + + permissions, err := am.permissionsManager.GetPermissionsByRole(ctx, user.Role) + if err == nil { + userWithPermissions.Permissions = permissions + } + + return userWithPermissions, nil } diff --git a/management/server/user_test.go b/management/server/user_test.go index 83c5ac49a..66bdc1683 100644 --- a/management/server/user_test.go +++ b/management/server/user_test.go @@ -13,7 +13,10 @@ import ( nbcache "github.com/netbirdio/netbird/management/server/cache" nbcontext "github.com/netbirdio/netbird/management/server/context" "github.com/netbirdio/netbird/management/server/permissions" + "github.com/netbirdio/netbird/management/server/permissions/modules" + "github.com/netbirdio/netbird/management/server/permissions/roles" "github.com/netbirdio/netbird/management/server/status" + "github.com/netbirdio/netbird/management/server/users" "github.com/netbirdio/netbird/management/server/util" nbpeer "github.com/netbirdio/netbird/management/server/peer" @@ -1020,90 +1023,6 @@ func TestDefaultAccountManager_ListUsers(t *testing.T) { assert.Equal(t, 2, regular) } -func TestDefaultAccountManager_ListUsers_DashboardPermissions(t *testing.T) { - testCases := []struct { - name string - role types.UserRole - limitedViewSettings bool - expectedDashboardPermissions string - }{ - { - name: "Regular user, no limited view settings", - role: types.UserRoleUser, - limitedViewSettings: false, - expectedDashboardPermissions: "limited", - }, - { - name: "Admin user, no limited view settings", - role: types.UserRoleAdmin, - limitedViewSettings: false, - expectedDashboardPermissions: "full", - }, - { - name: "Owner, no limited view settings", - role: types.UserRoleOwner, - limitedViewSettings: false, - expectedDashboardPermissions: "full", - }, - { - name: "Regular user, limited view settings", - role: types.UserRoleUser, - limitedViewSettings: true, - expectedDashboardPermissions: "blocked", - }, - { - name: "Admin user, limited view settings", - role: types.UserRoleAdmin, - limitedViewSettings: true, - expectedDashboardPermissions: "full", - }, - { - name: "Owner, limited view settings", - role: types.UserRoleOwner, - limitedViewSettings: true, - expectedDashboardPermissions: "full", - }, - } - - for _, testCase := range testCases { - t.Run(testCase.name, func(t *testing.T) { - store, cleanup, err := store.NewTestStoreFromSQL(context.Background(), "", t.TempDir()) - if err != nil { - t.Fatalf("Error when creating store: %s", err) - } - t.Cleanup(cleanup) - - account := newAccountWithId(context.Background(), mockAccountID, mockUserID, "") - account.Users["normal_user1"] = types.NewUser("normal_user1", testCase.role, false, false, "", []string{}, types.UserIssuedAPI) - account.Settings.RegularUsersViewBlocked = testCase.limitedViewSettings - delete(account.Users, mockUserID) - - err = store.SaveAccount(context.Background(), account) - if err != nil { - t.Fatalf("Error when saving account: %s", err) - } - - permissionsManager := permissions.NewManager(store) - am := DefaultAccountManager{ - Store: store, - eventStore: &activity.InMemoryEventStore{}, - permissionsManager: permissionsManager, - } - - users, err := am.ListUsers(context.Background(), mockAccountID) - if err != nil { - t.Fatalf("Error when checking user role: %s", err) - } - - assert.Equal(t, 1, len(users)) - - userInfo, _ := users[0].ToUserInfo(nil, account.Settings) - assert.Equal(t, testCase.expectedDashboardPermissions, userInfo.Permissions.DashboardView) - }) - } - -} - func TestDefaultAccountManager_ExternalCache(t *testing.T) { store, cleanup, err := store.NewTestStoreFromSQL(context.Background(), "", t.TempDir()) if err != nil { @@ -1654,121 +1573,154 @@ func TestDefaultAccountManager_GetCurrentUserInfo(t *testing.T) { tt := []struct { name string - accountId string - userId string + userAuth nbcontext.UserAuth expectedErr error - expectedResult *types.UserInfo + expectedResult *users.UserInfoWithPermissions }{ { name: "not found", - accountId: account1.Id, - userId: "not-found", + userAuth: nbcontext.UserAuth{AccountId: account1.Id, UserId: "not-found"}, expectedErr: status.NewUserNotFoundError("not-found"), }, { name: "not part of account", - accountId: account1.Id, - userId: "account2Owner", + userAuth: nbcontext.UserAuth{AccountId: account1.Id, UserId: "account2Owner"}, expectedErr: status.NewUserNotPartOfAccountError(), }, { name: "blocked", - accountId: account1.Id, - userId: "blocked-user", + userAuth: nbcontext.UserAuth{AccountId: account1.Id, UserId: "blocked-user"}, expectedErr: status.NewUserBlockedError(), }, { name: "service user", - accountId: account1.Id, - userId: "service-user", + userAuth: nbcontext.UserAuth{AccountId: account1.Id, UserId: "service-user"}, expectedErr: status.NewPermissionDeniedError(), }, { - name: "owner user", - accountId: account1.Id, - userId: "account1Owner", - expectedResult: &types.UserInfo{ - ID: "account1Owner", - Name: "", - Role: "owner", - AutoGroups: []string{}, - Status: "active", - IsServiceUser: false, - IsBlocked: false, - NonDeletable: false, - LastLogin: time.Time{}, - Issued: "api", - IntegrationReference: integration_reference.IntegrationReference{}, - Permissions: types.UserPermissions{ - DashboardView: "full", + name: "owner user", + userAuth: nbcontext.UserAuth{AccountId: account1.Id, UserId: "account1Owner"}, + expectedResult: &users.UserInfoWithPermissions{ + UserInfo: &types.UserInfo{ + ID: "account1Owner", + Name: "", + Role: "owner", + AutoGroups: []string{}, + Status: "active", + IsServiceUser: false, + IsBlocked: false, + NonDeletable: false, + LastLogin: time.Time{}, + Issued: "api", + IntegrationReference: integration_reference.IntegrationReference{}, }, + Permissions: mergeRolePermissions(roles.Owner), }, }, { - name: "regular user", - accountId: account1.Id, - userId: "regular-user", - expectedResult: &types.UserInfo{ - ID: "regular-user", - Name: "", - Role: "user", - Status: "active", - IsServiceUser: false, - IsBlocked: false, - NonDeletable: false, - LastLogin: time.Time{}, - Issued: "api", - IntegrationReference: integration_reference.IntegrationReference{}, - Permissions: types.UserPermissions{ - DashboardView: "limited", + name: "regular user", + userAuth: nbcontext.UserAuth{AccountId: account1.Id, UserId: "regular-user"}, + expectedResult: &users.UserInfoWithPermissions{ + UserInfo: &types.UserInfo{ + ID: "regular-user", + Name: "", + Role: "user", + Status: "active", + IsServiceUser: false, + IsBlocked: false, + NonDeletable: false, + LastLogin: time.Time{}, + Issued: "api", + IntegrationReference: integration_reference.IntegrationReference{}, }, + Permissions: mergeRolePermissions(roles.User), }, }, { - name: "admin user", - accountId: account1.Id, - userId: "admin-user", - expectedResult: &types.UserInfo{ - ID: "admin-user", - Name: "", - Role: "admin", - Status: "active", - IsServiceUser: false, - IsBlocked: false, - NonDeletable: false, - LastLogin: time.Time{}, - Issued: "api", - IntegrationReference: integration_reference.IntegrationReference{}, - Permissions: types.UserPermissions{ - DashboardView: "full", + name: "admin user", + userAuth: nbcontext.UserAuth{AccountId: account1.Id, UserId: "admin-user"}, + expectedResult: &users.UserInfoWithPermissions{ + UserInfo: &types.UserInfo{ + ID: "admin-user", + Name: "", + Role: "admin", + Status: "active", + IsServiceUser: false, + IsBlocked: false, + NonDeletable: false, + LastLogin: time.Time{}, + Issued: "api", + IntegrationReference: integration_reference.IntegrationReference{}, }, + Permissions: mergeRolePermissions(roles.Admin), }, }, { - name: "settings blocked regular user", - accountId: account2.Id, - userId: "settings-blocked-user", - expectedResult: &types.UserInfo{ - ID: "settings-blocked-user", - Name: "", - Role: "user", - Status: "active", - IsServiceUser: false, - IsBlocked: false, - NonDeletable: false, - LastLogin: time.Time{}, - Issued: "api", - IntegrationReference: integration_reference.IntegrationReference{}, - Permissions: types.UserPermissions{ - DashboardView: "blocked", + name: "settings blocked regular user", + userAuth: nbcontext.UserAuth{AccountId: account2.Id, UserId: "settings-blocked-user"}, + expectedResult: &users.UserInfoWithPermissions{ + UserInfo: &types.UserInfo{ + ID: "settings-blocked-user", + Name: "", + Role: "user", + Status: "active", + IsServiceUser: false, + IsBlocked: false, + NonDeletable: false, + LastLogin: time.Time{}, + Issued: "api", + IntegrationReference: integration_reference.IntegrationReference{}, }, + Permissions: mergeRolePermissions(roles.User), + Restricted: true, + }, + }, + + { + name: "settings blocked regular user child account", + userAuth: nbcontext.UserAuth{AccountId: account2.Id, UserId: "settings-blocked-user", IsChild: true}, + expectedResult: &users.UserInfoWithPermissions{ + UserInfo: &types.UserInfo{ + ID: "settings-blocked-user", + Name: "", + Role: "user", + Status: "active", + IsServiceUser: false, + IsBlocked: false, + NonDeletable: false, + LastLogin: time.Time{}, + Issued: "api", + IntegrationReference: integration_reference.IntegrationReference{}, + }, + Permissions: mergeRolePermissions(roles.User), + Restricted: false, + }, + }, + { + name: "settings blocked owner user", + userAuth: nbcontext.UserAuth{AccountId: account2.Id, UserId: "account2Owner"}, + expectedResult: &users.UserInfoWithPermissions{ + UserInfo: &types.UserInfo{ + ID: "account2Owner", + Name: "", + Role: "owner", + AutoGroups: []string{}, + Status: "active", + IsServiceUser: false, + IsBlocked: false, + NonDeletable: false, + LastLogin: time.Time{}, + Issued: "api", + IntegrationReference: integration_reference.IntegrationReference{}, + }, + Permissions: mergeRolePermissions(roles.Owner), }, }, } for _, tc := range tt { t.Run(tc.name, func(t *testing.T) { - result, err := am.GetCurrentUserInfo(context.Background(), tc.accountId, tc.userId) + result, err := am.GetCurrentUserInfo(context.Background(), tc.userAuth) if tc.expectedErr != nil { assert.Equal(t, err, tc.expectedErr) @@ -1780,3 +1732,17 @@ func TestDefaultAccountManager_GetCurrentUserInfo(t *testing.T) { }) } } + +func mergeRolePermissions(role roles.RolePermissions) roles.Permissions { + permissions := roles.Permissions{} + + for k := range modules.All { + if rolePermissions, ok := role.Permissions[k]; ok { + permissions[k] = rolePermissions + continue + } + permissions[k] = role.AutoAllowNew + } + + return permissions +} diff --git a/management/server/users/user.go b/management/server/users/user.go new file mode 100644 index 000000000..2f2788271 --- /dev/null +++ b/management/server/users/user.go @@ -0,0 +1,14 @@ +package users + +import ( + "github.com/netbirdio/netbird/management/server/permissions/roles" + "github.com/netbirdio/netbird/management/server/types" +) + +// Wrapped UserInfo with Role Permissions +type UserInfoWithPermissions struct { + *types.UserInfo + + Permissions roles.Permissions + Restricted bool +} diff --git a/relay/client/dialer/quic/quic.go b/relay/client/dialer/quic/quic.go index 7fd486f87..3fd48fb19 100644 --- a/relay/client/dialer/quic/quic.go +++ b/relay/client/dialer/quic/quic.go @@ -28,6 +28,16 @@ func (d Dialer) Dial(ctx context.Context, address string) (net.Conn, error) { return nil, err } + // Get the base TLS config + tlsClientConfig := quictls.ClientQUICTLSConfig() + + // Set ServerName to hostname if not an IP address + host, _, splitErr := net.SplitHostPort(quicURL) + if splitErr == nil && net.ParseIP(host) == nil { + // It's a hostname, not an IP - modify directly + tlsClientConfig.ServerName = host + } + quicConfig := &quic.Config{ KeepAlivePeriod: 30 * time.Second, MaxIdleTimeout: 4 * time.Minute, @@ -47,7 +57,7 @@ func (d Dialer) Dial(ctx context.Context, address string) (net.Conn, error) { return nil, err } - session, err := quic.Dial(ctx, udpConn, udpAddr, quictls.ClientQUICTLSConfig(), quicConfig) + session, err := quic.Dial(ctx, udpConn, udpAddr, tlsClientConfig, quicConfig) if err != nil { if errors.Is(err, context.Canceled) { return nil, err @@ -61,12 +71,29 @@ func (d Dialer) Dial(ctx context.Context, address string) (net.Conn, error) { } func prepareURL(address string) (string, error) { - if !strings.HasPrefix(address, "rel://") && !strings.HasPrefix(address, "rels://") { + var host string + var defaultPort string + + switch { + case strings.HasPrefix(address, "rels://"): + host = address[7:] + defaultPort = "443" + case strings.HasPrefix(address, "rel://"): + host = address[6:] + defaultPort = "80" + default: return "", fmt.Errorf("unsupported scheme: %s", address) } - if strings.HasPrefix(address, "rels://") { - return address[7:], nil + finalHost, finalPort, err := net.SplitHostPort(host) + if err != nil { + if strings.Contains(err.Error(), "missing port") { + return host + ":" + defaultPort, nil + } + + // return any other split error as is + return "", err } - return address[6:], nil + + return finalHost + ":" + finalPort, nil } diff --git a/release_files/install.sh b/release_files/install.sh index 49e313f2f..da5c613d5 100755 --- a/release_files/install.sh +++ b/release_files/install.sh @@ -224,16 +224,22 @@ check_use_bin_variable() { install_netbird() { if [ -x "$(command -v netbird)" ]; then - status_output=$(netbird status) - if echo "$status_output" | grep -q 'Management: Connected' && echo "$status_output" | grep -q 'Signal: Connected'; then - echo "NetBird service is running, please stop it before proceeding" - exit 1 - fi + status_output="$(netbird status 2>&1 || true)" - if [ -n "$status_output" ]; then - echo "NetBird seems to be installed already, please remove it before proceeding" - exit 1 - fi + if echo "$status_output" | grep -q 'failed to connect to daemon error: context deadline exceeded'; then + echo "Warning: could not reach NetBird daemon (timeout), proceeding anyway" + else + if echo "$status_output" | grep -q 'Management: Connected' && \ + echo "$status_output" | grep -q 'Signal: Connected'; then + echo "NetBird service is running, please stop it before proceeding" + exit 1 + fi + + if [ -n "$status_output" ]; then + echo "NetBird seems to be installed already, please remove it before proceeding" + exit 1 + fi + fi fi # Run the installation, if a desktop environment is not detected diff --git a/sharedsock/sock_linux.go b/sharedsock/sock_linux.go index 74ac6c163..1c22e7869 100644 --- a/sharedsock/sock_linux.go +++ b/sharedsock/sock_linux.go @@ -234,7 +234,7 @@ func (s *SharedSocket) read(receiver receiver) { } // ReadFrom reads packets received in the packetDemux channel -func (s *SharedSocket) ReadFrom(b []byte) (n int, addr net.Addr, err error) { +func (s *SharedSocket) ReadFrom(b []byte) (int, net.Addr, error) { var pkt rcvdPacket select { case <-s.ctx.Done(): @@ -263,8 +263,7 @@ func (s *SharedSocket) ReadFrom(b []byte) (n int, addr net.Addr, err error) { decodedLayers := make([]gopacket.LayerType, 0, 3) - err = parser.DecodeLayers(pkt.buf, &decodedLayers) - if err != nil { + if err := parser.DecodeLayers(pkt.buf, &decodedLayers); err != nil { return 0, nil, err } @@ -273,8 +272,8 @@ func (s *SharedSocket) ReadFrom(b []byte) (n int, addr net.Addr, err error) { Port: int(udp.SrcPort), } - copy(b, payload) - return int(udp.Length), remoteAddr, nil + n := copy(b, payload) + return n, remoteAddr, nil } // WriteTo builds a UDP packet and writes it using the specific IP version writer diff --git a/signal/peer/peer.go b/signal/peer/peer.go index ed2360d67..2252e877e 100644 --- a/signal/peer/peer.go +++ b/signal/peer/peer.go @@ -79,7 +79,7 @@ func (registry *Registry) Register(peer *Peer) { p, loaded := registry.Peers.LoadOrStore(peer.Id, peer) if loaded { pp := p.(*Peer) - log.Warnf("peer [%s] is already registered [new streamID %d, previous StreamID %d]. Will override stream.", + log.Tracef("peer [%s] is already registered [new streamID %d, previous StreamID %d]. Will override stream.", peer.Id, peer.StreamID, pp.StreamID) registry.Peers.Store(peer.Id, peer) return @@ -104,7 +104,7 @@ func (registry *Registry) Deregister(peer *Peer) { pp := p.(*Peer) if peer.StreamID < pp.StreamID { registry.Peers.Store(peer.Id, p) - log.Warnf("attempted to remove newer registered stream of a peer [%s] [newer streamID %d, previous StreamID %d]. Ignoring.", + log.Debugf("attempted to remove newer registered stream of a peer [%s] [newer streamID %d, previous StreamID %d]. Ignoring.", peer.Id, pp.StreamID, peer.StreamID) return } diff --git a/signal/server/signal.go b/signal/server/signal.go index 3cae7e860..bb11ee347 100644 --- a/signal/server/signal.go +++ b/signal/server/signal.go @@ -6,7 +6,6 @@ import ( "io" "time" - "github.com/netbirdio/signal-dispatcher/dispatcher" log "github.com/sirupsen/logrus" "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/metric" @@ -15,6 +14,8 @@ import ( "google.golang.org/grpc/status" gproto "google.golang.org/protobuf/proto" + "github.com/netbirdio/signal-dispatcher/dispatcher" + "github.com/netbirdio/netbird/signal/metrics" "github.com/netbirdio/netbird/signal/peer" "github.com/netbirdio/netbird/signal/proto" @@ -69,7 +70,7 @@ func NewServer(ctx context.Context, meter metric.Meter) (*Server, error) { // Send forwards a message to the signal peer func (s *Server) Send(ctx context.Context, msg *proto.EncryptedMessage) (*proto.EncryptedMessage, error) { - log.Debugf("received a new message to send from peer [%s] to peer [%s]", msg.Key, msg.RemoteKey) + log.Tracef("received a new message to send from peer [%s] to peer [%s]", msg.Key, msg.RemoteKey) if _, found := s.registry.Get(msg.RemoteKey); found { s.forwardMessageToPeer(ctx, msg) @@ -112,7 +113,7 @@ func (s *Server) ConnectStream(stream proto.SignalExchange_ConnectStreamServer) return err } - log.Debugf("Received a response from peer [%s] to peer [%s]", msg.Key, msg.RemoteKey) + log.Tracef("Received a response from peer [%s] to peer [%s]", msg.Key, msg.RemoteKey) _, err = s.dispatcher.SendMessage(stream.Context(), msg) if err != nil { @@ -149,7 +150,7 @@ func (s *Server) DeregisterPeer(p *peer.Peer) { } func (s *Server) forwardMessageToPeer(ctx context.Context, msg *proto.EncryptedMessage) { - log.Debugf("forwarding a new message from peer [%s] to peer [%s]", msg.Key, msg.RemoteKey) + log.Tracef("forwarding a new message from peer [%s] to peer [%s]", msg.Key, msg.RemoteKey) getRegistrationStart := time.Now() // lookup the target peer where the message is going to @@ -168,7 +169,7 @@ func (s *Server) forwardMessageToPeer(ctx context.Context, msg *proto.EncryptedM // forward the message to the target peer if err := dstPeer.Stream.Send(msg); err != nil { - log.Warnf("error while forwarding message from peer [%s] to peer [%s] %v", msg.Key, msg.RemoteKey, err) + log.Tracef("error while forwarding message from peer [%s] to peer [%s] %v", msg.Key, msg.RemoteKey, err) // todo respond to the sender? s.metrics.MessageForwardFailures.Add(ctx, 1, metric.WithAttributes(attribute.String(labelType, labelTypeError))) return diff --git a/upload-server/types/upload.go b/upload-server/types/upload.go index 35d003582..327c28e75 100644 --- a/upload-server/types/upload.go +++ b/upload-server/types/upload.go @@ -7,6 +7,8 @@ const ( ClientHeaderValue = "netbird" // GetURLPath is the path for the GetURL request GetURLPath = "/upload-url" + + DefaultBundleURL = "https://upload.debug.netbird.io" + GetURLPath ) // GetURLResponse is the response for the GetURL request