Draft: Music polishing 1 (only to get the rabbit running) #97
Conversation
* changed the config validation logic to not just fatal error on missing DB files but instead specifu what file is missing and what CLI command to use to create it. * implemented CLI commands to create DB files
we have the sidecar identity separate per sync mechanism. Note: This forces a sidecar that supports both API and DNS to use *separate* identities for these two services. Also note that while the identities must be separate, they mey refer to the same IP addresses (but not port, for obvious reasons). * mostly done with an implementation that synthesizes a new SVCB for the sidecar API (or DNS) identity. * not quite done with implementing the check SIG(0) key pair, generate key pair, publish public key, stuff. We have all that already, for the delegation sync. Question is whether to refactor or duplicate, as the requirements are slighly different.
sidecar identity names for both API and DNS mechanisms. * implemented automatic generation and publication of the needed TLSA, SVCB, KEY and A/AAAA records for both API and DNS mechanisms. Note that while these zones (at the identity names) are created automatically, this doesn't mean that there must be delegations pointing to them. It is mostly to get the correct and updated DNS records created. If it is unsuitable to have these as separate zones then the records are there so that they may be inserted into other zones. * A bunch of additional integration fallout between TDNS and MUSIC.
WalkthroughThis pull request introduces several changes across multiple components of the project. Key modifications include the addition of a Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 26
🧹 Outside diff range and nitpick comments (69)
agent/Makefile (1)
10-12: Consider declaring clean as a .PHONY targetTo ensure the clean target works even if a file named 'clean' exists in the directory, consider adding a .PHONY declaration.
+.PHONY: clean install + clean: rm -f $(PROG) rm -f *~ */*~server/Makefile (1)
11-13: LGTM! Consider some improvements for robustness.The clean target implementation is good but could be enhanced for better maintainability and portability.
Consider these improvements:
+.PHONY: clean clean: - rm -f $(PROG) - rm -f *~ */*~ + $(RM) $(PROG) + find . -type f -name '*~' -deleteChanges explained:
- Added
.PHONYto prevent conflicts with any potential file named 'clean'- Used
$(RM)instead ofrm -ffor better portability- Replaced multiple rm commands with a single
findcommand for better maintainabilitysidecar/Makefile (1)
10-12: LGTM with suggestions for improvementThe clean target correctly handles basic cleanup needs.
Consider these improvements:
+.PHONY: clean install + clean: rm -f $(PROG) - rm -f *~ */*~ + find . -type f -name '*~' -deleteThis would:
- Declare PHONY targets (prevents conflicts with same-named files)
- Use
findto clean backup files at any depthdog/Makefile (1)
11-13: Consider enhancing the clean target with best practices.A few suggestions to improve robustness and maintainability:
- Declare the target as .PHONY to prevent conflicts with same-named files
- Use $(RM) instead of rm -f for better portability
- Consider adding coverage for additional temporary files that might be generated during development
Here's the suggested implementation:
+.PHONY: clean clean: - rm -f $(PROG) - rm -f *~ */*~ + $(RM) $(PROG) + $(RM) *~ */*~ + $(RM) *.test *.out # Common Go test artifactssidecar-cli/Makefile (1)
15-18: Consider enhancing the clean target for better robustness.A few suggestions to improve the clean target:
- Mark it as .PHONY to ensure it runs even if a file named 'clean' exists
- Consider using find for recursive cleanup of temporary files
- Add cleaning of potential test artifacts
+.PHONY: clean clean: rm -f $(PROG) - rm -f *~ */*~ + find . -type f -name '*~' -delete + find . -type f -name '*.test' -delete + find . -type f -name 'coverage.out' -deletetdns/global.go (1)
26-26: Consider enhancing the field documentation.While the current comment indicates the CLI flag, it would be helpful to document the default behavior and impact of this setting.
- ShowHeaders bool // -H in various CLI commands + ShowHeaders bool // Controls header display in CLI commands (-H flag). When false, headers are hidden.sidecar-cli/cmd/music_cmds.go (1)
13-16: Consider enhancing the command descriptionWhile the command structure is correct, the Short description could be more informative about what MUSIC sub-commands actually do.
Consider something like:
- Short: "prefix cmd to reach all MUSIC sub-commands", + Short: "Manage MUSIC (Multi-Signer Coordination) operations including database, signers, and zones",tdns/cli/db_cmds.go (4)
15-16: Add documentation for the global variable.Consider adding a comment explaining the purpose and usage of
tdnsDbFile.+// tdnsDbFile holds the path to the TDNS database file, which can be set via command-line flag or configuration var tdnsDbFile string
17-20: Enhance command documentation.Consider adding a more detailed description using the
Longfield to provide usage examples and additional context.var DbCmd = &cobra.Command{ Use: "db", Short: "TDNS DB commands", + Long: `Database management commands for TDNS. + +Examples: + tdns db init -f /path/to/db + tdns db init --file /path/to/db`, }
26-31: Improve error message for missing configuration.The error message could be more helpful by suggesting how to resolve the issue.
- log.Fatalf("Error: TDNS DB file not specified in config nor on command line") + log.Fatalf("Error: TDNS DB file path is required. Specify it using --file flag or set 'db.file' in the configuration")
51-51: Enhance flag description.The flag description could provide more details about expected format and usage.
- dbInitCmd.Flags().StringVarP(&tdnsDbFile, "file", "f", "", "TDNS DB file") + dbInitCmd.Flags().StringVarP(&tdnsDbFile, "file", "f", "", "path to the TDNS database file (e.g., /path/to/tdns.db)")music/cmd/db_cmds.go (4)
5-5: Consider using a more descriptive package nameThe package name
mcmdis quite terse. Consider using a more descriptive name likemusiccmdordbcmdto better convey its purpose.
16-20: Add documentation for exported variables and commandsSince
DbCmdis exported, it should have a proper documentation comment. Also, consider documenting themusicDbFilevariable's purpose.+// musicDbFile specifies the path to the MUSIC database file var musicDbFile string + +// DbCmd represents the base command for database operations var DbCmd = &cobra.Command{
26-31: Improve configuration validationThe error message for missing configuration could be more helpful by indicating the expected configuration key and possible ways to set it.
- log.Fatalf("Error: MUSIC DB file not specified in config nor on command line") + log.Fatalf("Error: MUSIC DB file not specified. Use --file flag or set 'db.file' in config")
48-52: Add flag validationConsider adding validation for the
--fileflag value to ensure it meets any required format or constraints.func init() { DbCmd.AddCommand(dbInitCmd) dbInitCmd.Flags().StringVarP(&musicDbFile, "file", "f", "", "MUSIC DB file") + // Add validation + dbInitCmd.PreRunE = func(cmd *cobra.Command, args []string) error { + if f, _ := cmd.Flags().GetString("file"); f != "" { + if !strings.HasSuffix(f, ".db") { + return fmt.Errorf("database file must have .db extension") + } + } + return nil + } }tdns/cli/commands.go (4)
26-26: Consider consolidating display configuration settingsSimilar to how
showhdrwas moved totdns.Globals.ShowHeaders, consider moving these display-related flags to the global configuration for consistency.
28-29: Remove empty init functionThe
init()function is empty and serves no purpose. Consider removing it to improve code clarity.-func init() { -}
Line range hint
31-93: Standardize error handling across command functionsThere are inconsistencies in error handling between
SendCommandandSendCommandNG:
SendCommandNGlogs errors whileSendCommanddoesn't- Error wrapping patterns differ slightly
Consider standardizing the error handling approach across both functions.
Example standardization for
SendCommand:status, buf, err := tdns.Globals.Api.Post("/command", bytebuf.Bytes()) if err != nil { + log.Println("Error from Api Post:", err) return "", fmt.Errorf("error from api post: %v", err) }
Migration to
tdns.Globals.ShowHeadersis incompleteThe codebase still contains commented-out flag declarations using the old
showhdrvariable in multiple files:
tdns/cli/zone_cmds.go: Commented flag for "headers"tdns/cli/truststore_cmds.go: Commented flag for "showhdr"tdns/cli/zone_dsync_cmds.go: Commented flag for "showhdr"While the new
tdns.Globals.ShowHeadersis being used consistently in the actual code logic, these commented-out declarations should be removed to avoid confusion and maintain clean code.🔗 Analysis chain
Line range hint
1-93: Verify complete migration of header display configurationThe removal of
showhdrand migration totdns.Globals.ShowHeadersshould be verified across the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to showhdr rg 'showhdr' --type go # Search for new usage pattern to ensure consistent migration rg 'Globals\.ShowHeaders' --type goLength of output: 855
sidecar-cli/go.mod (1)
9-9: Dependency restructuring looks goodThe replacement of
libcliwithtdns/cliand corresponding module requirements are properly configured. However, consider the following suggestions for better version management:
- Document the reason for using pseudo-versions in a comment
- Consider using tagged versions once the module structure stabilizes
Also applies to: 15-15
music/cmd/root.go (2)
82-89: Maintain consistent error variable namingThe code uses both
erranderroras variable names for error handling. Stick to one convention, preferablyerr.err := json.NewEncoder(bytebuf).Encode(tmpapi) if err != nil { log.Fatalf("Error from json.NewEncoder: %v", err) } var prettyJSON bytes.Buffer -error := json.Indent(&prettyJSON, bytebuf.Bytes(), "", " ") -if error != nil { - log.Println("JSON parse error: ", error) +err = json.Indent(&prettyJSON, bytebuf.Bytes(), "", " ") +if err != nil { + log.Println("JSON parse error: ", err) }
90-90: Improve logging formatThe current Printf statement mixes multiple values in a single line. Consider structuring the log message for better readability.
-fmt.Printf("initApi: api connection to %s initialized (%s)\n:\napi: %s\n", baseurl, apikey, prettyJSON.String()) +fmt.Printf("InitApi: Initializing API connection\n") +fmt.Printf("BaseURL: %s\n", baseurl) +fmt.Printf("Auth: %s\n", apikey) +fmt.Printf("API Configuration:\n%s\n", prettyJSON.String())tdns/rr_dsync.go (2)
46-48: Document the purpose of numeric scheme representations.While the added numeric mappings are valid and match the scheme constants, please add documentation explaining why both string and numeric representations are supported. This will help future maintainers understand the design decision.
46-48: Consider refactoring the scheme mapping for better maintainability.The current implementation duplicates the mapping information. Consider refactoring to reduce maintenance overhead and improve clarity:
const ( - SchemeNotify = 1 - SchemeUpdate = 2 - SchemeAPI = 3 + SchemeNotify DsyncScheme = 1 + SchemeUpdate DsyncScheme = 2 + SchemeAPI DsyncScheme = 3 ) +// schemeInfo holds the string representation for each scheme +var schemeInfo = map[DsyncScheme]string{ + SchemeNotify: "NOTIFY", + SchemeUpdate: "UPDATE", + SchemeAPI: "API", +} + +// StringToScheme maps string representations to DsyncScheme values var StringToScheme = map[string]DsyncScheme{ - "NOTIFY": SchemeNotify, - "UPDATE": SchemeUpdate, - "API": SchemeAPI, - "1": SchemeNotify, - "2": SchemeUpdate, - "3": SchemeAPI, +} + +func init() { + // Initialize StringToScheme with both string and numeric representations + for scheme, str := range schemeInfo { + StringToScheme[str] = scheme + StringToScheme[strconv.Itoa(int(scheme))] = scheme + } } -var SchemeToString = map[DsyncScheme]string{ - SchemeNotify: "NOTIFY", - SchemeUpdate: "UPDATE", - SchemeAPI: "API", +// SchemeToString returns the string representation of a scheme +func SchemeToString(scheme DsyncScheme) string { + if str, ok := schemeInfo[scheme]; ok { + return str + } + return fmt.Sprintf("UNKNOWN(%d)", scheme) }This refactoring:
- Uses a single source of truth for scheme information
- Automatically generates both string and numeric mappings
- Adds type safety to scheme constants
- Improves error handling for unknown schemes
tdns/rr_msigner.go (3)
24-36: LGTM! Consider adding documentation.The renaming from
SchemetoMechanismimproves clarity, and the removal of the port field simplifies the protocol. Consider adding documentation comments to explain:
- The purpose of the MSIGNER record type
- The valid states and mechanisms
- The rationale for removing the port field
67-93: Consider enhancing error messages.The changes are consistent with the removal of the port parameter. Consider improving error messages by:
- Including the expected format in the error message
- Using a constant for the expected parameter count
Example:
- return errors.New("MSIGNER requires a state, a sync mechanism and a target") + return fmt.Errorf("MSIGNER record requires 3 fields: <state> <mechanism> <target>, got %d fields", len(txt))
107-107: Consider adding mechanism value validation.The packing/unpacking changes are correct, but consider adding validation to ensure the mechanism value is within the expected range.
Example for Unpack:
rd.Mechanism = MsignerMechanism(tmp) + if rd.Mechanism != MSignMechanismDNS && rd.Mechanism != MSignMechanismAPI { + return off, fmt.Errorf("invalid mechanism value: %d", tmp) + }Also applies to: 142-142
music/desec_mgr.go (2)
28-44: Add documentation for the inactive state behavior.Consider adding a comment block explaining:
- The purpose of the inactive state
- Expected behavior when deSEC is inactive
- Configuration requirements
Add this documentation above the conditional block:
+// When deSEC service is inactive (signers.desec.active = false), the manager +// will continue to accept fetch/update requests to prevent blocking clients, +// but these requests will be logged and ignored. This allows the system to +// operate in a degraded mode without deSEC integration.
35-35: Enhance logging messages with more context.The current log messages could be more descriptive to aid in debugging and monitoring.
Consider this enhancement:
- log.Println("deSEC Manager: fetch request received. Ignoring.") + log.Printf("deSEC Manager: fetch request received but service is inactive - request ignored") - log.Println("deSEC Manager: update request received. Ignoring.") + log.Printf("deSEC Manager: update request received but service is inactive - request ignored")Also applies to: 37-37
tdns/cli/zone_dsync_cmds.go (2)
173-173: Remove commented-out code.Instead of keeping commented-out code, consider removing it entirely. The version control history will preserve this information if needed later.
Apply this diff:
- // zoneDsyncCmd.PersistentFlags().BoolVarP(&showhdr, "showhdr", "H", false, "Show headers")
Line range hint
28-41: Consider refactoring duplicated error handling.There's a repeated pattern of error handling across multiple command handlers. Consider extracting this into a helper function to reduce code duplication and improve maintainability.
Consider adding a helper function like this:
func handleCommandError(err error, resp tdns.ZoneDsyncResponse) { if err != nil { fmt.Printf("Error: %s\n", err.Error()) os.Exit(1) } if resp.Error { fmt.Printf("Error from tdns-server: %s\n", resp.ErrorMsg) os.Exit(1) } if resp.Msg != "" { fmt.Printf("%s\n", resp.Msg) } }Then use it in each command handler:
- if err != nil { - fmt.Printf("Error: %s\n", err.Error()) - os.Exit(1) - } - if resp.Error { - fmt.Printf("Error from tdns-server: %s\n", resp.ErrorMsg) - os.Exit(1) - } - if resp.Msg != "" { - fmt.Printf("%s\n", resp.Msg) - } + handleCommandError(err, resp)Also applies to: 84-97, 134-147, 164-177
tdns/cli/truststore_cmds.go (3)
Line range hint
134-144: Remove commented debug print statement and approve header changes.The header display logic using
tdns.Globals.ShowHeaderslooks good. However, there's a commented-out debug print statement// fmt.Printf("Known child SIG(0) keys:\n")that should be removed for cleaner code.
Line range hint
119-213: Improve error handling consistency inSig0TrustMgmt.The function mixes different error handling patterns:
- Some errors are returned to the caller
- Others directly call
os.Exit(1)This makes error handling unpredictable and harder to test. Consider:
- Consistently returning errors instead of calling
os.Exit- Let the calling command handle the errors uniformly
Example refactor for one section:
if subcommand == "add" { mapkey := fmt.Sprintf("%s::%d", tdns.Globals.Zonename, keyid) if _, ok := tr.ChildSig0keys[mapkey]; ok { - fmt.Printf("Error: key with name %s and keyid %d is already known.\n", tdns.Globals.Zonename, keyid) - os.Exit(1) + return fmt.Errorf("key with name %s and keyid %d is already known", tdns.Globals.Zonename, keyid) }
Line range hint
164-171: Consider additional security validations for SIG(0) keys.While the code validates key ownership and prevents duplicates, consider adding:
- Validation of the key's algorithm to ensure it meets security requirements
- Checks for minimum key strength
- Validation of key expiration dates if applicable
tdns/refreshengine.go (3)
Line range hint
82-92: Add context cancellation to zone refresh goroutine.The goroutine launched for zone refresh lacks proper context cancellation, which could lead to goroutine leaks if the refresh operation hangs.
Consider passing a context to control the goroutine lifecycle:
- go func(zd *ZoneData) { + go func(ctx context.Context, zd *ZoneData) { + select { + case <-ctx.Done(): + return + default: updated, err := zd.Refresh(Globals.Verbose, Globals.Debug, zr.Force) if err != nil { log.Printf("RefreshEngine: Error from zone refresh(%s): %v", zone, err) } if updated { log.Printf("Zone %s was updated via refresh operation", zd.ZoneName) } - }(zd) + } + }(conf.Context, zd)
Line range hint
196-218: Refactor duplicated SOA serial reset logic.The SOA serial reset logic is duplicated between the channel and ticker cases. This could lead to maintenance issues and inconsistencies.
Extract the common logic into a helper function:
+func (zd *ZoneData) handleZoneUpdate(resetSoaSerial bool) { + if resetSoaSerial { + zd.CurrentSerial = uint32(time.Now().Unix()) + log.Printf("RefreshEngine: %s updated from upstream. Resetting serial to unixtime: %d", + zd.ZoneName, zd.CurrentSerial) + } + zd.NotifyDownstreams() +}Then use it in both places:
- if resetSoaSerial { - zd.CurrentSerial = uint32(time.Now().Unix()) - log.Printf("RefreshEngine: %s updated from upstream. Resetting serial to unixtime: %d", - zone, zd.CurrentSerial) - } - zd.NotifyDownstreams() + zd.handleZoneUpdate(resetSoaSerial)Also applies to: 234-245
Line range hint
258-277: Improve error handling in FindSoaRefresh.The function ignores potential errors from
GetSOAand doesn't properly handle edge cases.Consider improving the error handling:
func FindSoaRefresh(zd *ZoneData) (uint32, error) { var refresh uint32 - soa, _ := zd.GetSOA() + soa, err := zd.GetSOA() + if err != nil { + return 0, fmt.Errorf("failed to get SOA record: %w", err) + } + if soa == nil { + return 0, fmt.Errorf("no SOA record found for zone %s", zd.ZoneName) + } if soa != nil { refresh = soa.Refresh }sidecar/main.go (4)
Line range hint
19-67: Clean up signal handling and resource management.Several issues need attention in the signal handling implementation:
- Duplicate log message at the end of mainloop
- Potential race condition in signal handling
- Missing cleanup of goroutines on exit
Apply these changes to improve the implementation:
func mainloop(conf *tdns.Config, mconf *music.Config, appMode string) { log.Printf("mainloop: starting") exit := make(chan os.Signal, 1) signal.Notify(exit, syscall.SIGINT, syscall.SIGTERM) hupper := make(chan os.Signal, 1) signal.Notify(hupper, syscall.SIGHUP) + defer signal.Stop(exit) + defer signal.Stop(hupper) var err error var all_zones []string var wg sync.WaitGroup wg.Add(1) go func() { + defer wg.Done() for { select { case <-exit: log.Println("mainloop: Exit signal received. Cleaning up.") - wg.Done() + return case <-hupper: log.Println("mainloop: SIGHUP received. Forcing refresh of all configured zones.") all_zones, err = tdns.ParseZones(conf, conf.Internal.RefreshZoneCh, true) if err != nil { log.Fatalf("Error parsing zones: %v", err) } else { log.Printf("mainloop: SIGHUP received. Forcing refresh of %d configured zones.", len(all_zones)) } case <-conf.Internal.APIStopCh: log.Println("mainloop: Stop command received. Cleaning up.") - wg.Done() + return } } }() wg.Wait() mconf.Internal.TokViper.WriteConfig() fmt.Printf("mainloop: saved state of API tokens to disk\n") - fmt.Println("mainloop: leaving signal dispatcher") - fmt.Println("mainloop: leaving signal dispatcher") + fmt.Println("mainloop: exiting") }
129-130: Consider adding configuration validation.After loading the MUSIC configuration, it would be beneficial to validate the loaded configuration before proceeding.
Consider adding validation:
music.LoadMusicConfig(&mconf, tconf.AppMode, false) +if err := validateMusicConfig(&mconf); err != nil { + log.Fatalf("Invalid MUSIC configuration: %v", err) +}
Line range hint
200-250: Improve error handling and resource management for API clients.The desecapi initialization and channel setup could benefit from better error handling and resource management.
Consider these improvements:
+ ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + rootcafile := viper.GetString("common.rootCA") - desecapi, err := music.DesecSetupClient(rootcafile, music.CliConf.Verbose, music.CliConf.Debug) + desecapi, err := music.DesecSetupClient(ctx, rootcafile, music.CliConf.Verbose, music.CliConf.Debug) if err != nil { log.Fatalf("Error from DesecSetupClient: %v\n", err) } + if desecapi == nil { + log.Fatalf("DesecSetupClient returned nil API client") + } desecapi.TokViper = music.TokVip rldu := music.Updaters["rldesec-api"] + if rldu == nil { + log.Fatalf("Updater 'rldesec-api' not found") + } rldu.SetChannels(mconf.Internal.DesecFetch, mconf.Internal.DesecUpdate) rldu.SetApi(*desecapi)
Line range hint
251-266: Add graceful shutdown mechanism.The goroutine management at the end of main could benefit from a more structured shutdown approach using context.
Consider implementing a graceful shutdown:
- var done = make(chan struct{}, 1) + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() - go music.DbUpdater(&mconf) - go music.DeSECmgr(&mconf, done) - go music.DdnsMgr(&mconf, done) - go music.FSMEngine(&mconf, done) + go music.DbUpdater(ctx, &mconf) + go music.DeSECmgr(ctx, &mconf) + go music.DdnsMgr(ctx, &mconf) + go music.FSMEngine(ctx, &mconf) - mainloop(&tconf, &mconf, appMode) + mainloop(ctx, &tconf, &mconf, appMode)tdns/cli/keystore_cmds.go (3)
Line range hint
318-332: Consider improving the header implementationThe current implementation could be enhanced in the following ways:
- Extract the header string as a constant to avoid duplication
- Consider making the key record truncation length configurable
- Add a flag to show full key records when needed
+ const ( + SIG0_HEADER = "Signer|State|KeyID|Algorithm|PrivKey|KEY Record" + KEY_TRUNCATE_LENGTH = 50 + ) if tdns.Globals.ShowHeaders { - out = append(out, "Signer|State|KeyID|Algorithm|PrivKey|KEY Record") + out = append(out, SIG0_HEADER) } if len(tr.Sig0keys) > 0 { for k, v := range tr.Sig0keys { tmp := strings.Split(k, "::") - tmplist = append(tmplist, fmt.Sprintf("%s|%s|%s|%v|%v|%.50s...\n", + keystr := v.Keystr + if len(keystr) > KEY_TRUNCATE_LENGTH && !tdns.Globals.ShowFullKey { + keystr = keystr[:KEY_TRUNCATE_LENGTH] + "..." + } + tmplist = append(tmplist, fmt.Sprintf("%s|%s|%s|%v|%v|%s\n", - tmp[0], v.State, tmp[1], v.Algorithm, v.PrivateKey, v.Keystr)) + tmp[0], v.State, tmp[1], v.Algorithm, v.PrivateKey, keystr))
Line range hint
419-424: Ensure consistency in header display logicThe DNSSEC key listing implementation differs from the SIG(0) implementation in several ways:
- Headers are prepended here but appended in the SIG(0) section
- An extra "Known DNSSEC key pairs:" message is printed before the table
- The empty list message format differs from the SIG(0) section
- fmt.Printf("Known DNSSEC key pairs:\n") for k, v := range tr.Dnskeys { tmp := strings.Split(k, "::") out = append(out, fmt.Sprintf("%s|%s|%s|%d|%s|%s|%.50s...\n", tmp[0], v.State, tmp[1], v.Flags, v.Algorithm, v.PrivateKey, v.Keystr)) } sort.Strings(out) + var finalOut []string if tdns.Globals.ShowHeaders { - out = append([]string{"Signer|State|KeyID|Flags|Algorithm|PrivKey|DNSKEY Record"}, out...) + finalOut = append(finalOut, "Signer|State|KeyID|Flags|Algorithm|PrivKey|DNSKEY Record") } + finalOut = append(finalOut, out...) - fmt.Printf("%s\n", columnize.SimpleFormat(out)) + fmt.Printf("%s\n", columnize.SimpleFormat(finalOut))
Line range hint
1-476: Consider architectural improvements for better maintainability and securitySeveral architectural improvements could enhance the code:
- Avoid using
os.Exit()in library functions. Instead, return errors and let the caller decide how to handle them.- Consider implementing proper logging with different levels instead of mixing
fmt.Printfandlog.Printf.- Add validation for key states and algorithms.
- Implement secure handling of private key data (clearing memory after use).
Example of improved error handling:
func Sig0KeyMgmt(cmd string) error { if !isValidCommand(cmd) { - fmt.Printf("Unknown keystore command: \"%s\"\n", cmd) - os.Exit(1) + return fmt.Errorf("invalid keystore command: %q", cmd) } // ... rest of the function } +func isValidCommand(cmd string) bool { + validCommands := map[string]bool{ + "list": true, + "add": true, + "import": true, + "generate": true, + "delete": true, + "setstate": true, + } + return validCommands[cmd] +}tdns/parseconfig.go (1)
198-212: Consider implementing structured logging.The current implementation mixes direct logging with error handling. Consider introducing a structured logging interface that would:
- Provide consistent log levels
- Enable better log aggregation
- Support structured metadata
- Allow easier filtering and monitoring
Example structured logging libraries for Go:
zaplogruszerologtdns/dnsutils.go (2)
355-356: Consider removing duplicate error logging.The same error is being both logged and returned, which could lead to duplicate error messages in logs. Consider removing the
log.Printfsince the error is already being logged by the caller on line 341.- log.Printf("ParseZoneFromReader: Zone %s: Error: SOA: %v", zd.ZoneName, soa_rrset) return false, 0, fmt.Errorf("ParseZoneFromReader: Zone %s: Error: SOA: %v", zd.ZoneName, soa_rrset)
Line range hint
1-600: Consider improving error handling patterns and concurrency safety.While reviewing the changes, I noticed some broader concerns:
Error Handling Pattern:
- Some functions return errors directly while others log and return
- Consider adopting a consistent error handling pattern across the file
- Consider using error wrapping (introduced in Go 1.13) for better error context
Concurrency Safety:
- The file uses
cmap.New[OwnerData]()for concurrent operations- However, some operations like
GetOwnerfollowed by accessing the data aren't atomic- Consider adding mutex protection for these compound operations
Would you like me to provide specific examples of how to implement these improvements?
tdns/delegation_utils.go (1)
569-571: Consider standardizing nil apex data handlingThe functions handle nil apex data (initial zone load) differently:
DnskeysChangedNGreturns(true, nil)MsignerChangedreturns(true, &mss, nil)This inconsistency could be confusing for maintainers.
Consider standardizing the behavior across both functions, possibly by:
- Having both return a status struct
- Adding comments explaining why they behave differently
- Creating a shared helper function for nil apex handling
Also applies to: 619-621
tdns/zone_utils.go (3)
995-1007: Consider making zone template configurable.The zone template contains several hard-coded values that might need to be customized:
- TTL values
- SOA parameters
- IP address (currently using TEST-NET-1 range)
- Serial number format
Consider moving this template to a configuration file or making these values configurable parameters.
1030-1039: Remove commented-out debug code.The commented-out debug code should be removed as it adds noise to the codebase. If debugging is needed, consider using proper logging levels instead.
1022-1025: Enhance error handling with more context.The error handling could be more descriptive by including the zone name in the error message.
- return nil, fmt.Errorf("failed to read zone data: %v", err) + return nil, fmt.Errorf("failed to read zone data for zone '%s': %v", zonename, err)tdns/ops_svcb.go (4)
27-47: Remove commented-out code to enhance readabilityThe code between lines 27 and 47 is commented out and not used. Removing it will improve code readability and maintainability.
You can remove the commented code to clean up the function.
13-13: Remove unused parameterportfrom function signatureThe parameter
portinPublishSvcbRRis not used in the function implementation. Consider removing it to avoid confusion and potential maintenance issues.Apply this diff to remove the unused parameter:
-func (zd *ZoneData) PublishSvcbRR(name string, port uint16, value []dns.SVCBKeyValue) error { +func (zd *ZoneData) PublishSvcbRR(name string, value []dns.SVCBKeyValue) error {Remember to update any calls to this function accordingly.
87-90: Consider adding timeout to DNS queriesTo handle slow or unresponsive DNS servers gracefully, it's recommended to set a timeout for DNS queries.
Here's how you can set a timeout on the DNS client:
import ( "fmt" "log" + "time" "github.com/miekg/dns" ) ... c := new(dns.Client) + c.Timeout = 5 * time.Second r, _, err := c.Exchange(m, clientConfig.Servers[0]+":53")Make sure to import the
"time"package at the top of the file.
82-85: Consider making DNS resolver configurable instead of hardcoding/etc/resolv.confRelying directly on
/etc/resolv.confmight not be portable across different environments. Consider allowing the DNS resolver configuration to be specified or providing default servers in case the file is missing.This enhances the portability and flexibility of the function across different systems.
music/syncengine.go (3)
140-142: Limit Potentially Verbose Logging of Sidecar InformationLogging the entire
sidecarsmap may lead to overly verbose logs, especially if the map contains a large number of entries or sensitive information.Consider summarizing the information or logging only essential details to keep logs concise and informative.
- log.Printf("MusicSyncEngine: received heartbeats from these sidecars: %+v (the expected result)", sidecars) + log.Printf("MusicSyncEngine: received heartbeats from %d sidecars (the expected result)", len(sidecars))
331-336: Use Logger Instead of Printing to Standard OutputPrinting directly to standard output (
fmt.Printf) is not recommended for production code. Use thelogpackage to ensure consistent logging practices.Apply this diff to replace
fmt.Printfwithlog.Printf:- fmt.Printf("Received response: %s\n", string(body)) + log.Printf("Received response: %s", string(body))
216-238: Implement Sending HELLO Message to New SidecarsThe code currently logs that it is scheduling a HELLO message but does not actually send it. Implement the necessary logic to send HELLO messages to new sidecars.
Apply this diff to call the
SendHellomethod:// Add code to send HELLO message here +if err := sidecars[remoteMechanism][remoteSidecar].SendHello(); err != nil { + log.Printf("Failed to send HELLO to sidecar %s (%s): %v", remoteSidecar, tdns.MsignerMechanismToString[remoteMechanism], err) +}tdns/queryresponder.go (2)
406-406: Typo correction in commentThe comment has been corrected from "catcheverything" to "catch everything" for improved readability.
Line range hint
164-166: Consider replacinglog.Fatalfwith an error return for consistent error handlingIn
QueryResponder, when retrieving the apex data fails, consider returning the error instead of callinglog.Fatalf. This change promotes graceful error handling and aligns with the pattern used inApexResponder.Apply this diff to replace the fatal log with an error return:
func (zd *ZoneData) QueryResponder(w dns.ResponseWriter, r *dns.Msg, qname string, qtype uint16, dnssec_ok bool, kdb *KeyDB) error { apex, err := zd.GetOwner(zd.ZoneName) if err != nil { - log.Fatalf("QueryResponder: failed to get apex data for zone %s", zd.ZoneName) + return fmt.Errorf("QueryResponder: failed to get apex data for zone %s: %v", zd.ZoneName, err) }Remember to import the
fmtpackage if it's not already imported:import ( "log" + "fmt" "sort" "strings"music/config.go (2)
107-107: Review the commented validation tag forDbConf.FileThe validation tag
validate:"file"forDbConf.Fileis commented out. If file validation is necessary, consider re-enabling it or ensuring validation occurs elsewhere.
308-536: Refactor duplicated code inLoadSidecarConfigfor maintainabilityThe logic for handling
mconf.Sidecar.Apiandmconf.Sidecar.Dnsis largely duplicated. Refactoring this code into reusable functions would improve maintainability and reduce the chance of introducing bugs when making future changes.Consider extracting common functionality into helper functions, such as:
- A function to validate and process the identity.
- A function to publish records (TLSA, SVCB, KEY, addresses).
- A function to handle errors consistently.
tdns/delegation_sync.go (1)
165-170: Refactor duplicated algorithm selection codeThe code for fetching and parsing the key generation algorithm and handling the default case is duplicated in both
DelegationSyncSetupandMusicSig0KeyPrep. Consider refactoring this code into a separate helper function to improve maintainability and reduce code duplication.Also applies to: 193-197
tdns/zone_updater.go (6)
Line range hint
317-371: Ensure thread safety when modifying zone dataThe use of
zd.mu.Lock()and deferredzd.mu.Unlock()provides thread safety when modifying theZoneData. However, in the deferred function, you're callingzd.BumpSerial()after releasing the lock. To maintain thread safety, consider callingzd.BumpSerial()before unlocking to ensure that serial updates are also protected by the mutex.Apply this diff to adjust the lock scope:
func (zd *ZoneData) ApplyChildUpdateToZoneData(ur UpdateRequest, kdb *KeyDB) (bool, error) { zd.mu.Lock() defer func() { - zd.mu.Unlock() zd.BumpSerial() + zd.mu.Unlock() }() // Function logic... }
Line range hint
591-645: Maintain lock duringBumpSerialcall to ensure consistencySimilar to the earlier comment, in
ApplyZoneUpdateToZoneData, the deferred function releases the lock before callingzd.BumpSerial(). To prevent potential race conditions, ensure thatzd.BumpSerial()is called while the lock is still held.Apply this diff to adjust the lock scope:
func (zd *ZoneData) ApplyZoneUpdateToZoneData(ur UpdateRequest, kdb *KeyDB) (bool, error) { zd.mu.Lock() defer func() { - zd.mu.Unlock() zd.BumpSerial() + zd.mu.Unlock() }() // Function logic... }
Line range hint
886-888: Remove commented code or clarify its purposeThe comment
// dump.P(ddata)and// dump.P(dss)at the end of theZoneUpdateChangesDelegationDataNGfunction suggests leftover debugging statements. If they're no longer needed, consider removing them to keep the code clean.
Line range hint
728-747: Handle potential errors when retrieving delegation dataIn the
ZoneUpdateChangesDelegationDataNGfunction, the error returned byzd.DelegationData()is assigned but not checked. Ensure that you handle this error to prevent potentialnildereferences or unintended behavior ifddataisnil.Apply this diff to handle the error:
func (zd *ZoneData) ZoneUpdateChangesDelegationDataNG(ur UpdateRequest) (DelegationSyncStatus, error) { log.Printf("*** Enter ZUCDDNGNG(). ur:\n%+v", ur) var dss = DelegationSyncStatus{ ZoneName: zd.ZoneName, Time: time.Now(), InSync: true, } defer func() { log.Printf("********* ZUCDDNGNG: returning") }() - ddata, err := zd.DelegationData() + ddata, err := zd.DelegationData() + if err != nil { + log.Printf("Error retrieving delegation data: %v", err) + return dss, err + } apex, err := zd.GetOwner(zd.ZoneName) if err != nil { return dss, err }
Line range hint
707-715: Avoid redundant logging of variable namesIn the log statement:
log.Printf("ZoneUpdateChangesDelegationData: checking action: %s", rr.String)
rr.Stringis referencing the method itself, not calling it. It should berr.String()to correctly output the string representation of the RR.Apply this diff to correct the log statement:
-log.Printf("ZoneUpdateChangesDelegationData: checking action: %s", rr.String) +log.Printf("ZoneUpdateChangesDelegationData: checking action: %s", rr.String())
Line range hint
813-831: Ensure thread safety when accessingzd.UpdatePolicyAccess to
zd.UpdatePolicyshould be protected by the mutexzd.muto ensure thread safety. Currently, there's no lock acquired when checkingzd.UpdatePolicy.Zone.RRtypes[rrtype]. Consider acquiring a read lock if concurrent reads are allowed, or a write lock if necessary.Apply this diff to lock the mutex before accessing
zd.UpdatePolicy:+zd.mu.Lock() defer func() { + zd.mu.Unlock() log.Printf("********* ZUCDDNGNG: returning") }() ddata, err := zd.DelegationData()Or ensure that the function that calls this code already has acquired the necessary locks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (40)
agent/Makefile(1 hunks)cli/Makefile(1 hunks)cli/cmd/shared_cmds.go(1 hunks)dog/Makefile(1 hunks)music/cmd/db_cmds.go(1 hunks)music/cmd/root.go(2 hunks)music/cmd/tdns_cmds.go.foo(0 hunks)music/config.go(6 hunks)music/desec_mgr.go(1 hunks)music/syncengine.go(6 hunks)server/Makefile(1 hunks)sidecar-cli/Makefile(1 hunks)sidecar-cli/cmd/music_cmds.go(2 hunks)sidecar-cli/cmd/root.go(1 hunks)sidecar-cli/cmd/tdns_cmds.go(1 hunks)sidecar-cli/go.mod(1 hunks)sidecar/Makefile(1 hunks)sidecar/apiserver.go(3 hunks)sidecar/main.go(1 hunks)tdns/cli/commands.go(1 hunks)tdns/cli/db_cmds.go(1 hunks)tdns/cli/keystore_cmds.go(2 hunks)tdns/cli/truststore_cmds.go(2 hunks)tdns/cli/zone_cmds.go(2 hunks)tdns/cli/zone_dsync_cmds.go(2 hunks)tdns/delegation_sync.go(4 hunks)tdns/delegation_utils.go(2 hunks)tdns/dnsutils.go(1 hunks)tdns/global.go(1 hunks)tdns/ops_a_aaaa.go(1 hunks)tdns/ops_svcb.go(1 hunks)tdns/parseconfig.go(2 hunks)tdns/queryresponder.go(4 hunks)tdns/refreshengine.go(1 hunks)tdns/rr_dsync.go(1 hunks)tdns/rr_msigner.go(5 hunks)tdns/rr_print.go(8 hunks)tdns/zone_updater.go(4 hunks)tdns/zone_utils.go(2 hunks)utils/Makefile.common(1 hunks)
💤 Files with no reviewable changes (1)
- music/cmd/tdns_cmds.go.foo
✅ Files skipped from review due to trivial changes (1)
- cli/cmd/shared_cmds.go
🧰 Additional context used
🪛 golangci-lint (1.61.0)
music/cmd/root.go
81-81: SA1026: trying to marshal unsupported type func(req *net/http.Request, via []*net/http.Request) error, via x.Client.CheckRedirect
(staticcheck)
tdns/delegation_sync.go
255-255: printf: non-constant format string in call to (*log.Logger).Printf
(govet)
🔇 Additional comments (53)
agent/Makefile (1)
10-12: LGTM! Clean target implementation follows make conventions.
The clean target correctly handles the removal of both the compiled binary and editor backup files.
server/Makefile (1)
11-13: Verify consistency across all Makefiles
Let's ensure the clean target implementation is consistent across all Makefiles mentioned in the summary.
✅ Verification successful
Clean target implementation is consistent across all component Makefiles
All component Makefiles (agent, cli, dog, sidecar-cli, sidecar, server) implement identical clean targets that:
- Remove the program binary:
rm -f $(PROG) - Remove editor backup files:
rm -f *~ */*~
The root Makefile correctly orchestrates cleaning all components by recursively calling their clean targets.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check clean target implementation across all Makefiles
# Expected: Similar clean target implementations in agent/, cli/, dog/, sidecar-cli/, and sidecar/ directories
# Find all Makefiles and show their clean targets
fd Makefile --type f --exec sh -c 'echo "=== {} ==="; awk "/^clean:/,/^[^\t]/" "{}"'
Length of output: 341
Script:
#!/bin/bash
# Let's check the complete content of each Makefile's clean target including their commands
fd Makefile --type f --exec sh -c 'echo "=== {} ==="; awk "/^clean:/,/^[^[:space:]|clean:]/" "{}" | grep -v "^$"'
Length of output: 726
sidecar/Makefile (1)
10-12: Verify consistency with other Makefiles
Since this is part of a larger cleanup effort across multiple Makefiles, let's ensure consistency.
✅ Verification successful
Clean target implementation is consistent across component Makefiles
The clean target in sidecar/Makefile follows the same pattern used consistently across all component Makefiles (cli/, server/, dog/, sidecar-cli/, agent/), which remove the program binary ($(PROG)) and cleanup backup files (*~ /~). The root Makefile appropriately orchestrates cleanup by calling clean targets of its subcomponents.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check clean target implementation across all Makefiles
# Expected: Similar clean target patterns
# Find all Makefiles and check their clean targets
fd Makefile --type f --exec sh -c 'echo "=== {} ==="; grep -A 2 "^clean:" "{}" || true; echo'
Length of output: 591
dog/Makefile (1)
11-13: LGTM! Clean target implementation is functional.
The clean target correctly removes the compiled binary and temporary files.
cli/Makefile (2)
Line range hint 1-15: Verify common Makefile inclusions
Let's ensure there are no conflicts or redundancies with the included Makefile.common.
✅ Verification successful
No conflicts found in Makefile inclusions
The verification shows that:
Makefile.commondefines common build targets and variables but does not define acleantarget- The
cleantarget incli/Makefileis unique and doesn't conflict with the included common Makefile - The common targets (build, version.go, test, lint) complement the specific targets in cli/Makefile
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check contents of the common Makefile
cat ../utils/Makefile.common 2>/dev/null || echo "Common Makefile not found in expected location"
# Check if clean target is defined in other included Makefiles
fd Makefile.common | xargs grep -l "clean:"
Length of output: 177
Script:
#!/bin/bash
# Let's search for Makefile.common in the entire repository
fd -t f "Makefile.common"
# Let's also check the parent directory structure
ls -la ../ 2>/dev/null || echo "Cannot access parent directory"
# Let's check the current directory structure to understand the context
ls -R . 2>/dev/null || echo "Cannot list current directory"
Length of output: 4820
Script:
#!/bin/bash
# Now that we found Makefile.common, let's check its contents
cat utils/Makefile.common
# Let's also check if there are any other Makefiles with clean targets
fd Makefile | xargs grep -l "clean:"
Length of output: 1226
Line range hint 6-7: Verify SQLite dependency comment
The commented code suggests a transition to using SQLite in the CLI tool. Let's verify if this comment is still relevant and if the SQLite dependency is properly handled.
sidecar-cli/Makefile (1)
15-18: LGTM! Clean target implementation is functional.
The clean target correctly removes the compiled program and temporary files.
tdns/global.go (1)
26-26: LGTM! The ShowHeaders field is well-placed and properly exported.
The addition of this field aligns well with the CLI restructuring effort, centralizing the header display configuration.
sidecar-cli/cmd/tdns_cmds.go (2)
7-7: Verify complete migration from libcli to tdns/cli
The import path has been updated from libcli to tdns/cli. Let's ensure this migration is complete across the codebase.
✅ Verification successful
Migration from libcli to tdns/cli is complete
The search results confirm that:
- No references to the old
libclipath exist in the codebase - The new
tdns/clipath is consistently used across all relevant files including go.mod files and command implementations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to the old libcli path
# and verify the new tdns/cli path is consistently used
echo "Checking for any remaining references to old libcli path..."
rg "github\.com/johanix/tdns/libcli"
echo "Verifying consistent usage of new tdns/cli path..."
rg "github\.com/johanix/tdns/tdns/cli"
Length of output: 859
11-38: Well-structured command organization with clear source references
The command registration block is well-organized with helpful source file comments. Each command is logically grouped by functionality, making the code maintainable and easy to navigate.
✅ Verification successful
Let me generate another verification script to check the command definitions with a corrected pattern:
The previous script had all files present, but we need to verify the actual command definitions. The new script uses both ast-grep and rg with context to ensure we catch the command definitions properly.
All command definitions verified and properly located
The command registration block accurately reflects the source files and their corresponding command definitions. All commands are properly defined in their respective files under the tdns/cli/ directory with appropriate Use and Short descriptions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all referenced command files exist in the new structure
# and that each command is properly defined
echo "Checking existence of referenced command files..."
for file in db_cmds.go start_cmds.go zone_cmds.go ddns_cmds.go debug_cmds.go keystore_cmds.go truststore_cmds.go dsync_cmds.go config_cmds.go; do
fd -p "tdns/cli/$file$"
done
echo "Verifying command definitions..."
for cmd in DbCmd PingCmd DaemonCmd ZoneCmd DdnsCmd DelCmd DebugCmd KeystoreCmd TruststoreCmd DsyncDiscoveryCmd ConfigCmd; do
ast-grep --pattern "var $cmd = &cobra.Command{$$$}"
done
Length of output: 4035
Script:
#!/bin/bash
# Description: Verify command definitions with proper pattern matching
echo "Verifying command definitions with proper pattern..."
for cmd in DbCmd PingCmd DaemonCmd ZoneCmd DdnsCmd DelCmd DebugCmd KeystoreCmd TruststoreCmd DsyncDiscoveryCmd ConfigCmd; do
echo "Checking $cmd..."
ast-grep --pattern "var $cmd = &cobra.Command{$$$}"
rg "var $cmd = &cobra.Command" -A 2
done
Length of output: 5368
sidecar-cli/cmd/music_cmds.go (3)
6-10: LGTM!
The imports are clean, necessary, and follow standard Go conventions.
Line range hint 23-45: Verify the existence of all referenced command files
Let's ensure all the referenced command files exist in the music/cmd directory.
✅ Verification successful
All referenced command files exist and contain their corresponding command definitions
All command files referenced in the imports are present in the music/cmd directory, and each file contains its corresponding command variable definition. The only minor difference is that status.go is actually named cmdStatus.go, but this doesn't affect the functionality since the import comments correctly reference the source location.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence of all referenced command files
# Check for the existence of all referenced command files
echo "Checking for command files..."
for file in status.go deseclogin.go process.go show.go signer.go signergroup.go test.go zone.go; do
if fd -p "music/cmd/$file"; then
echo "✓ Found $file"
else
echo "✗ Missing $file"
fi
done
# Verify each file contains its corresponding Cmd variable
echo -e "\nVerifying command definitions..."
rg "var (Status|Desec|Process|Show|Signer|SignerGroup|Test|Zone)Cmd" "music/cmd/"
Length of output: 2197
20-22: Verify the new location of DbCmd
The comment indicates DbCmd has been moved to db_cmds.go. Let's verify this change.
✅ Verification successful
DbCmd is properly defined and imported
The verification confirms:
- DbCmd is properly defined in
music/cmd/db_cmds.goas a Cobra command - The import statement in
sidecar-cli/cmd/music_cmds.gocorrectly references the package asmcmd - The command registration follows the same pattern as other commands in the file
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and proper definition of DbCmd in its new location
# Check if db_cmds.go exists and contains DbCmd
rg -l "var DbCmd" "music/cmd/db_cmds.go"
# Verify there are no remaining references to DbCmd in the old location
rg "var DbCmd" "music/cmd/status.go"
Length of output: 98
Script:
#!/bin/bash
# Let's verify the actual content of db_cmds.go to ensure DbCmd is properly defined
rg "DbCmd" "music/cmd/db_cmds.go" -A 3
# Also check the import statement in the CLI file to ensure proper package reference
rg "mcmd" "sidecar-cli/cmd/music_cmds.go" -B 2
Length of output: 961
tdns/cli/db_cmds.go (2)
1-14: LGTM! Package and imports are well-structured.
The package name aligns with the directory structure, and imports are appropriate for the CLI functionality.
22-46: Verify uniqueness of database initialization logic.
Let's ensure this initialization logic isn't duplicated elsewhere in the codebase.
✅ Verification successful
Let me gather more information to verify if there are other database initialization commands in the codebase.
Based on the search results, I can now provide a final response:
Database initialization logic is properly isolated.
The database initialization logic is unique to each component and properly isolated:
- TDNS DB initialization in
tdns/cli/db_cmds.go - MUSIC DB initialization in
music/cmd/db_cmds.go
Each component has its own dedicated initialization with appropriate error handling and configuration checks. The implementations are similar but separate, which is the correct approach for independent components.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar database initialization patterns
rg -g '!{tdns/cli/db_cmds.go}' -A 5 'os\.Create.*\.db' .
rg -g '!{tdns/cli/db_cmds.go}' -A 5 'db[[:space:]]+init' .
# Search for other cobra commands that might handle database operations
ast-grep --pattern 'var $_ = &cobra.Command{
Use: $db,
$$$
}'
Length of output: 831
Script:
#!/bin/bash
# Search for all cobra commands in the codebase
ast-grep --pattern 'var $_ = &cobra.Command{
Use: "init",
$$$
}'
# Search for database file creation patterns
rg -g '!{tdns/cli/db_cmds.go}' -A 5 'os\.Create.*file' .
# Look for similar database command structures
fd -e go -x grep -l "db.*Command" {}
Length of output: 100363
music/cmd/db_cmds.go (1)
22-46: Verify consistent database initialization patterns
Let's check if similar database initialization patterns exist elsewhere in the codebase to ensure consistent implementation and security measures.
✅ Verification successful
Database initialization pattern is consistent and secure
The implementation follows a consistent pattern across the codebase. Both music/cmd/db_cmds.go and tdns/cli/db_cmds.go use identical security measures:
- Configuration validation through viper
- Existence check using
os.Stat - Safe file creation with proper error handling
- Proper file closure with
defer
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar database initialization patterns
rg -l "os\.Create.*\.db"
rg -l "db\.file"
ast-grep --pattern 'os.OpenFile($_, $_, $_)'
Length of output: 244
Script:
#!/bin/bash
# Let's examine the database initialization patterns in these files
rg -A 10 "db\.file" music/cmd/db_cmds.go tdns/cli/db_cmds.go
rg -A 10 "os\.Create" music/cmd/db_cmds.go tdns/cli/db_cmds.go
Length of output: 2427
sidecar-cli/cmd/root.go (2)
Line range hint 28-28: Verify initialization function ownership.
Given the pattern of moving functionality from mcmd to tdns, consider if mcmd.InitConfig and mcmd.InitApi should also be migrated to the tdns package for consistency.
Let's check the implementation and usage of these init functions:
35-36: Consider completing the flag migration pattern.
The flags are currently split between two different packages:
tdns.Globals: Verbose, Debug, ShowHeaders, Zonenamemcmd: CfgFile, Signername, Sgroupname
This mixed usage could lead to confusion. Consider migrating the remaining flags (CfgFile, Signername, Sgroupname) to tdns.Globals for consistency.
Let's verify if other files in the codebase follow a similar pattern:
tdns/ops_a_aaaa.go (1)
42-48: Verify concurrent safety of zone updates
Both methods send updates through KeyDB.UpdateQ without clear documentation about thread safety. Please verify:
- Is
KeyDB.UpdateQproperly initialized? - Are there any race conditions possible when multiple goroutines call these methods?
- Is there a mechanism to handle channel closure?
Let's check the usage and initialization:
Also applies to: 69-74
music/cmd/root.go (1)
8-9: LGTM: Import statements are appropriate
The added imports are necessary for the new JSON formatting functionality.
tdns/rr_msigner.go (3)
49-56: LGTM! Consistent renaming.
The map renames are consistent with the type changes and maintain the correct mappings.
171-171: LGTM! Copy method updated correctly.
The Copy method changes are consistent with the struct updates and maintain proper deep copying behavior.
63-63: Verify impact of string format change.
The string representation change is consistent with the struct updates. However, we should verify if any code depends on parsing this string format.
✅ Verification successful
Let me check the Parse implementation to understand if it depends on the string format.
Let me try with ripgrep to check the Parse implementation.
String format change is safe to proceed
The Parse implementation has been updated to match the new string format, removing the port parsing and expecting only 3 components (state, mechanism, target) instead of 4. The changes in String() and Parse() methods are consistent with each other.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for code that might be parsing MSIGNER string output
rg -l 'MSIGNER.*String\(\)' | xargs rg 'Split|Parse|scan'
Length of output: 194
Script:
#!/bin/bash
# Check the Parse implementation in MSIGNER
ast-grep --pattern 'func (rd *MSIGNER) Parse($$$) {
$$$
}'
Length of output: 62
Script:
#!/bin/bash
# Check the Parse implementation in MSIGNER
rg -A 20 "func \(rd \*MSIGNER\) Parse" tdns/rr_msigner.go
Length of output: 704
music/desec_mgr.go (1)
28-44: Implementation looks solid and well-structured!
The new inactive state handling is well-implemented with proper:
- Channel management
- Resource cleanup
- Error handling
- Concurrency safety
The code maintains the existing functionality while gracefully handling the inactive state.
sidecar/apiserver.go (3)
9-9: LGTM: Import additions are appropriate
The new imports (fmt and net) are correctly added to support the enhanced address handling functionality.
Also applies to: 11-11
110-116: LGTM: Improved configuration management
The switch from direct viper calls to structured configuration via mconf improves code organization and maintainability.
129-130: Consider improving address handling robustness
While using net.JoinHostPort is the correct approach, consider these improvements:
- Add error handling for port conversion
- Consider validating the address format before using it
Here's a suggested improvement:
- addr := net.JoinHostPort(address, fmt.Sprintf("%d", port))
- log.Fatal(http.ListenAndServeTLS(addr, certFile, keyFile, router))
+ addr := net.JoinHostPort(address, fmt.Sprintf("%d", port))
+ if err := http.ListenAndServeTLS(addr, certFile, keyFile, router); err != nil {
+ log.Printf("Failed to start server on %s: %v", addr, err)
+ return
+ }Additionally, verify that the address format is valid:
tdns/cli/zone_dsync_cmds.go (1)
53-56: LGTM! Verify global header configuration usage.
The switch to using tdns.Globals.ShowHeaders aligns with the centralized configuration approach.
Let's verify the consistent usage of this global configuration across other CLI files:
✅ Verification successful
Migration to global header configuration is complete and consistent
The verification confirms that:
- No local
showhdrflag declarations remain in the CLI files tdns.Globals.ShowHeadersis consistently used across all CLI command files:- zone_cmds.go
- truststore_cmds.go
- keystore_cmds.go
- zone_dsync_cmds.go
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of ShowHeaders across CLI files
# Expect: All CLI commands should use tdns.Globals.ShowHeaders instead of local flags
# Check for any remaining local showhdr declarations
rg -l 'showhdr.*BoolVarP' 'tdns/cli/'
# Verify the new global usage
rg 'Globals\.ShowHeaders' 'tdns/cli/'
Length of output: 366
tdns/cli/truststore_cmds.go (1)
106-106: Remove commented-out code instead of leaving it as a comment.
The showhdr flag has been replaced with tdns.Globals.ShowHeaders. Clean up by removing the commented-out code.
Let's verify if there are any remaining references to the old showhdr flag:
✅ Verification successful
Remove all commented-out showhdr flag declarations across the codebase
The search results confirm that the showhdr flag has been globally replaced with tdns.Globals.ShowHeaders, which is now properly defined in tdns/global.go and set via the root command's persistent flags. However, there are several leftover commented-out declarations that should be removed:
tdns/cli/zone_dsync_cmds.go:// zoneDsyncCmd.PersistentFlags().BoolVarP(&showhdr, "showhdr", "H", false, "Show headers")tdns/cli/truststore_cmds.go:// TruststoreCmd.PersistentFlags().BoolVarP(&showhdr, "showhdr", "H", false, "Show column headers")tdns/cli/zone_cmds.go:// zoneListCmd.Flags().BoolVarP(&showhdr, "headers", "H", false, "Show column headers")
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to the showhdr flag
rg --type go 'showhdr|ShowHeaders' -A 3 -B 3
Length of output: 5212
tdns/refreshengine.go (1)
126-129:
Review error handling strategy for zone refresh operations.
The current error handling approach has several concerns:
- Using
continueafter the error skips important zone setup operations (SetupZoneSigning, SetupZoneSync) that might still be necessary even if the refresh failed. - There's a commented out
returnstatement that should be removed to maintain code cleanliness. - The error is logged but not propagated to the caller, which might need to know about the failure.
Consider restructuring the error handling:
- log.Printf("RefreshEngine: Error from zone refresh(%s): %v", zone, err)
- continue // cannot do much else
- // return // terminate goroutine
+ log.Printf("RefreshEngine: Error from zone refresh(%s): %v", zone, err)
+ // Continue with setup operations but track the refresh failure
+ zd.Options[OptRefreshFailed] = trueLet's verify the impact of skipping these setup operations:
✅ Verification successful
The error handling strategy is appropriate, no changes needed
After analyzing the implementation of SetupZoneSigning and SetupZoneSync, the current error handling with continue is actually correct because:
SetupZoneSigningis a no-op for zones that:- Don't have online signing enabled
- Don't allow updates
- Are agent zones
SetupZoneSyncis a no-op for zones that don't have delegation sync options enabled
If zone refresh fails, it's safer to skip these setup operations as they depend on the zone being in a valid state. The commented-out return can stay as it documents a considered alternative.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are other places in the codebase that rely on these setup operations
rg "SetupZoneSigning|SetupZoneSync" --type go
Length of output: 1237
Script:
#!/bin/bash
# Let's check the implementation of these setup functions to understand their criticality
ast-grep --pattern 'func (zd *ZoneData) SetupZoneSigning($$$) {
$$$
}'
ast-grep --pattern 'func (zd *ZoneData) SetupZoneSync($$$) {
$$$
}'
Length of output: 146
Script:
#!/bin/bash
# Let's check the content of these functions using ripgrep for better context
rg -A 10 "func \(zd \*ZoneData\) SetupZoneSigning" --type go
rg -A 10 "func \(zd \*ZoneData\) SetupZoneSync" --type go
Length of output: 1476
tdns/cli/zone_cmds.go (1)
252-252: LGTM: Clean transition to global header control
The change to use tdns.Globals.ShowHeaders aligns with the centralized header display management approach.
sidecar/main.go (1)
Line range hint 1-266: Verify secure handling of sensitive data.
The application handles sensitive operations and data:
- API tokens stored in TokViper
- DNS update operations
- Database operations
Please ensure:
- API tokens are stored securely
- All network communications use TLS
- Database access is properly secured
Run the following to check for potential security issues:
tdns/rr_print.go (3)
17-17: LGTM: Function renames follow Go naming conventions
The renaming of functions from *RRPrint to Print*RR follows Go's idiomatic verb-noun order, improving code readability and maintainability.
Also applies to: 49-49, 79-79, 99-99, 119-119
185-185: LGTM: Function calls correctly updated
All function calls within ZoneTransferPrint have been properly updated to use the new function names while maintaining the same functionality.
Also applies to: 192-192, 195-195, 200-200, 211-211
316-316: LGTM: Function calls correctly updated
All function calls within PrintRR have been properly updated to use the new function names while maintaining the same functionality.
Also applies to: 322-322, 324-324, 326-326, 328-328, 330-330
tdns/parseconfig.go (1)
198-212: LGTM! Improved error handling for database initialization.
The changes properly handle the database file verification with clear error messages and user guidance.
tdns/dnsutils.go (1)
341-341: LGTM! Enhanced error logging with zone context.
The addition of zone name in the error message improves debugging capabilities, especially in multi-zone environments.
tdns/delegation_utils.go (2)
619-619: LGTM: Consistent logging improvement
The log message prefix change from "DDCNG" to "MsignerChanged" improves clarity and maintains consistency with other functions.
569-571:
Verify the impact of changed return value during initial zone load
The function now returns true instead of false when old apex data is nil (during initial zone load). This is a significant behavioral change that could affect systems relying on the previous behavior.
Consider:
- Adding a comment explaining why this behavior was changed
- Ensuring all callers are updated to handle the new behavior
- Updating relevant test cases
tdns/zone_utils.go (3)
798-802: LGTM! Efficient early return for zones not requiring sync.
The added check improves performance by avoiding unnecessary processing when delegation sync is not required. The logging is clear and helpful for debugging.
1017-1017:
Review security implications of enabling updates by default.
Setting OptAllowUpdates: true by default could have security implications. Consider making this configurable or disabled by default unless explicitly required.
✅ Verification successful
Let me analyze the findings and generate additional verification scripts to understand the configuration and security controls better.
Let me analyze the sample configuration to understand the default settings better.
Based on the comprehensive analysis of the codebase and configuration samples, I can now provide the final response:
Setting OptAllowUpdates=true by default in CreateAutoZone is safe with existing security controls
The default setting is acceptable because:
- Updates are protected by SIG(0) authentication (no unauthenticated updates)
- The server has a built-in policy framework controlling which updates are allowed
- Configuration templates show that update policies are explicitly defined with restricted RR types
- The server validates signatures and enforces local policy before accepting updates
- The code includes proper security checks throughout the update handling path
The review comment's concern is addressed by the following security measures:
- Updates require cryptographic signatures (SIG(0))
- Update policies must be explicitly configured to specify allowed record types
- The server includes a truststore for validating update signatures
- Multiple validation checks are performed before processing updates
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for other instances where OptAllowUpdates is used
rg "OptAllowUpdates" -A 5 -B 5
Length of output: 12875
Script:
#!/bin/bash
# Check for configuration files and examples to understand default settings
fd -e yaml -e yml -e conf -e example --exec rg -l "allow-updates|update-policy|update_policy"
# Check for documentation about update policies
fd "README|SECURITY|CONFIGURATION" --exec cat {} \;
Length of output: 10547
Script:
#!/bin/bash
# Check the sample configuration for update policy settings
cat server/tdns-zones.sample.yaml
# Look for any tests or examples showing the intended behavior
fd -e go -e yaml test update
Length of output: 2539
1041-1041: Verify thread safety of global Zones map.
The Zones.Set() operation on a global map should be verified for thread safety.
✅ Verification successful
Let me gather more information about the Zones type definition and its implementation.
Let me check what cmap package is being used since it appears to be a concurrent map implementation.
Let me check the implementation of the cmap package to verify its thread safety.
Let me try a different approach to find the cmap implementation.
Thread safety is properly handled through concurrent-map implementation
The Zones variable is using github.com/orcaman/concurrent-map/v2 which is a well-known, production-ready implementation of a thread-safe map in Go. This package provides built-in synchronization mechanisms, making all operations on the map thread-safe by default. The Set() and Get() operations being used are automatically protected against concurrent access.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for concurrent access patterns to Zones map
rg "Zones\.(Set|Get)" -A 5 -B 5
# Check for mutex usage around Zones operations
ast-grep --pattern 'type Zones struct {
$$$
mu sync.Mutex
$$$
}'
Length of output: 13092
Script:
#!/bin/bash
# Check the Zones type definition
ast-grep --pattern 'type Zones struct {
$$$
}'
# Look for sync.Map usage
rg "sync\.Map" -A 2 -B 2
# Check the Zones variable declaration
rg "var Zones" -A 2 -B 2
Length of output: 186
Script:
#!/bin/bash
# Check imports for cmap
rg "import.*cmap" -A 5 -B 5
# Look for cmap package definition
fd cmap.go
# If found, check its implementation
rg "New\[.*\]" -A 5 -B 5
Length of output: 6031
Script:
#!/bin/bash
# Look for cmap package source
fd -e go -x cat {} \; | rg -A 10 -B 10 "type ConcurrentMap"
# Check imports to find which cmap package is used
rg "^import.*\".*cmap.*\""
Length of output: 91
Script:
#!/bin/bash
# Look for go.mod to find dependencies
cat go.mod
# Search for cmap in vendor directory if it exists
fd -t f cmap vendor/
# Look for local cmap implementation
fd cmap.go
# Check imports in global.go where Zones is defined
rg -A 5 "^import" tdns/global.go
Length of output: 320
music/syncengine.go (4)
214-215: Consider Error Handling for Unrecognized Record Types
When processing RRs, if the type is unrecognized, the code logs the issue but does not handle it further. Consider whether additional error handling or reporting is necessary.
Ensure that unrecognized RR types are correctly handled to prevent unexpected behavior.
289-292: Validate s.Identity Before Using in TLSA Lookup
Ensure that s.Identity is a valid domain name before using it in the tdns.LookupTLSA function to prevent potential errors.
Run the following script to check the validity of s.Identity values:
57-60: 🛠️ Refactor suggestion
Handle Missing Mechanisms in sidecarId Map
When accessing sidecarId[remoteMechanism], ensure that the remoteMechanism key exists in the sidecarId map to prevent potential runtime panics due to missing keys.
Apply this diff to add a check for the existence of the mechanism before accessing the map:
if remoteSidecar == sidecarId[remoteMechanism] {
+ // Check if the mechanism exists in sidecarId
+ if _, ok := sidecarId[remoteMechanism]; !ok {
+ log.Printf("Unknown mechanism: %s", tdns.MsignerMechanismToString[remoteMechanism])
+ continue
+ }
// we don't need to send a hello to ourselves
log.Printf("MaybeSendHello: remoteSidecar [%s][%s] is ourselves (%s), no need to talk to ourselves",
tdns.MsignerMechanismToString[remoteMechanism], remoteSidecar, sidecarId[remoteMechanism])
continue
}Likely invalid or redundant comment.
26-26: Ensure Consistent Initialization of the Mechanism Field
The addition of the Mechanism field to the Sidecar struct is appropriate for differentiating between API and DNS mechanisms. Ensure that all instances where Sidecar is instantiated are updated to initialize this new field to prevent potential nil dereferences or uninitialized values.
Run the following script to find all instantiations of Sidecar and verify that Mechanism is properly initialized:
✅ Verification successful
The Mechanism field is properly initialized in all instances
Based on the code analysis, the Mechanism field is correctly initialized in the only instance where a Sidecar is created. The instantiation at music/syncengine.go explicitly sets the Mechanism field using the remoteMechanism value obtained from the DNS record. No other instantiations of the Sidecar struct were found in the codebase, and the field is properly used in switch statements and map access operations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all instantiations of Sidecar and check for Mechanism initialization.
# Search for Sidecar instantiations missing 'Mechanism' initialization
ast-grep --pattern $'&Sidecar {
$_,
$$$
}' --lang go --json | jq '.matches[] | select(.code | contains("Mechanism") | not) | .code'
Length of output: 204
Script:
#!/bin/bash
# Let's first find all files containing 'Sidecar' struct instantiations
rg -l "Sidecar\s*{" --type go
# Then look for actual instantiations with context
rg "Sidecar\s*{" --type go -B 2 -A 5
Length of output: 242
Script:
#!/bin/bash
# Let's first find all files containing 'Sidecar' struct instantiations
rg -l "Sidecar {" --type go
# Then look for actual instantiations with context
rg "Sidecar {" --type go -B 2 -A 5
# Also search for any direct references to the Mechanism field
rg "\.Mechanism" --type go -B 2 -A 2
Length of output: 2783
Script:
#!/bin/bash
# Let's try a different pattern to find Sidecar instantiations
rg "&Sidecar" --type go -B 2 -A 5
# Also search for Sidecar constructor functions
rg "func.*[Nn]ew.*Sidecar" --type go -B 2 -A 5
Length of output: 589
tdns/queryresponder.go (3)
42-42: Good practice: Return error when apex data retrieval fails
Returning the error allows the caller to handle the failure appropriately and enables upstream error handling.
92-92: Support for dns.TypeSVCB added to supported DNS types
This enhancement allows the responder to handle SVCB DNS records, broadening the range of supported DNS record types.
351-352: Expanded support for additional DNS record types
The addition of dns.TypeSVCB, dns.TypeHTTPS, dns.TypeTLSA, and dns.TypeCAA allows the QueryResponder to handle these DNS record types, enhancing the responder's functionality.
music/config.go (1)
15-15:
Ensure 'slices' package is correctly imported
The slices package is used, but it requires Go version 1.21 or later. If you're using an earlier version of Go, you'll need to import slices from golang.org/x/exp/slices.
Run the following script to check your Go version and verify the import:
✅ Verification successful
The slices package import is correctly used
Based on the go.mod files, all modules in the project are using Go version 1.21 or later (ranging from 1.21 to 1.23.2), which fully supports the built-in slices package. The usage of slices.Contains throughout the codebase is appropriate and doesn't require any changes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the Go version and verify the 'slices' package import.
# Check Go version. Expect: Go version >= 1.21
go version
# Search for the import statement of 'slices'. Expect: Correct import path.
rg '^import\s+"slices"$' music/
Length of output: 139
Script:
#!/bin/bash
# Let's check the go.mod file to determine the Go version and verify the import
fd "go.mod$" --exec cat {}
# Let's also check the actual usage of slices package
rg "slices\." --type go
Length of output: 26425
tdns/zone_updater.go (3)
64-64: Verify the impact of reducing the ticker interval from 60s to 10s
The runQueueTicker interval has been reduced from 60 seconds to 10 seconds. This change increases the frequency at which deferred updates are processed. Please ensure that this does not negatively affect system performance or introduce any unintended side effects due to the increased processing rate.
80-81: Improved handling of unknown zone names for deferred updates
The condition now correctly excludes "DEFERRED-UPDATE" commands when checking for unknown zone names. This adjustment ensures that deferred updates are processed even if the zone name is not recognized, which is appropriate for deferred operations.
226-243: Correct implementation of item removal during slice iteration
Changing to an index-based loop allows for the safe removal of items from the deferredUpdates slice while iterating. The logic correctly increments the index i only when an item is not removed, ensuring that all deferred updates are processed without skipping any elements.
| if _, err := os.Stat(musicDbFile); err == nil { | ||
| fmt.Printf("Warning: MUSIC DB file '%s' already exists.\n", musicDbFile) | ||
| } else if os.IsNotExist(err) { | ||
| file, err := os.Create(musicDbFile) | ||
| if err != nil { | ||
| log.Fatalf("Error creating MUSIC DB file '%s': %v", musicDbFile, err) | ||
| } | ||
| defer file.Close() | ||
| fmt.Printf("MUSIC DB file '%s' created successfully.\n", musicDbFile) | ||
| } else { | ||
| log.Fatalf("Error checking MUSIC DB file '%s': %v", musicDbFile, err) | ||
| } |
There was a problem hiding this comment.
Add security measures and improve error handling
Several improvements could be made to the file creation logic:
- Set explicit file permissions
- Validate the file path
- Use atomic operations to prevent race conditions
Here's a suggested implementation:
- if _, err := os.Stat(musicDbFile); err == nil {
- fmt.Printf("Warning: MUSIC DB file '%s' already exists.\n", musicDbFile)
- } else if os.IsNotExist(err) {
- file, err := os.Create(musicDbFile)
+ // Validate file path
+ dir := filepath.Dir(musicDbFile)
+ if _, err := os.Stat(dir); err != nil {
+ log.Fatalf("Invalid directory for database file: %v", err)
+ }
+
+ // Attempt to create file with specific permissions
+ file, err := os.OpenFile(musicDbFile, os.O_RDWR|os.O_CREATE|os.O_EXCL, 0600)
+ if err != nil {
+ if os.IsExist(err) {
+ fmt.Printf("Warning: MUSIC DB file '%s' already exists.\n", musicDbFile)
+ return
+ }
if err != nil {
log.Fatalf("Error creating MUSIC DB file '%s': %v", musicDbFile, err)
}
- defer file.Close()
- fmt.Printf("MUSIC DB file '%s' created successfully.\n", musicDbFile)
- } else {
- log.Fatalf("Error checking MUSIC DB file '%s': %v", musicDbFile, err)
}
+ defer file.Close()
+ fmt.Printf("MUSIC DB file '%s' created successfully.\n", musicDbFile)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if _, err := os.Stat(musicDbFile); err == nil { | |
| fmt.Printf("Warning: MUSIC DB file '%s' already exists.\n", musicDbFile) | |
| } else if os.IsNotExist(err) { | |
| file, err := os.Create(musicDbFile) | |
| if err != nil { | |
| log.Fatalf("Error creating MUSIC DB file '%s': %v", musicDbFile, err) | |
| } | |
| defer file.Close() | |
| fmt.Printf("MUSIC DB file '%s' created successfully.\n", musicDbFile) | |
| } else { | |
| log.Fatalf("Error checking MUSIC DB file '%s': %v", musicDbFile, err) | |
| } | |
| // Validate file path | |
| dir := filepath.Dir(musicDbFile) | |
| if _, err := os.Stat(dir); err != nil { | |
| log.Fatalf("Invalid directory for database file: %v", err) | |
| } | |
| // Attempt to create file with specific permissions | |
| file, err := os.OpenFile(musicDbFile, os.O_RDWR|os.O_CREATE|os.O_EXCL, 0600) | |
| if err != nil { | |
| if os.IsExist(err) { | |
| fmt.Printf("Warning: MUSIC DB file '%s' already exists.\n", musicDbFile) | |
| return | |
| } | |
| if err != nil { | |
| log.Fatalf("Error creating MUSIC DB file '%s': %v", musicDbFile, err) | |
| } | |
| } | |
| defer file.Close() | |
| fmt.Printf("MUSIC DB file '%s' created successfully.\n", musicDbFile) |
| case tdns.MSignMechanismAPI: | ||
| // Create the SidecarHelloPost struct | ||
| helloPost := SidecarHelloPost{ | ||
| SidecarId: s.Identity, | ||
| Addresses: s.Addresses, | ||
| Port: s.Port, | ||
| } | ||
|
|
||
| // Lookup the TLSA record for the target sidecar | ||
| tlsarrset, err := tdns.LookupTLSA(s.Identity) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to lookup TLSA record: %v", err) | ||
| } | ||
| // Encode the struct as JSON | ||
| jsonData, err := json.Marshal(helloPost) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to marshal SidecarHelloPost: %v", err) | ||
| } | ||
|
|
||
| // Use the TLSA record to authenticate the remote end securely | ||
| // (This is a simplified example, in a real implementation you would need to configure the TLS client with the TLSA record) | ||
| tlsConfig := &tls.Config{ | ||
| VerifyPeerCertificate: func(rawCerts [][]byte, verifiedChains [][]*x509.Certificate) error { | ||
| for _, rawCert := range rawCerts { | ||
| cert, err := x509.ParseCertificate(rawCert) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to parse certificate: %v", err) | ||
| } | ||
| if cert.Subject.CommonName != s.Identity { | ||
| return fmt.Errorf("unexpected certificate common name (should have been %s)", s.Identity) | ||
| } | ||
| // Lookup the TLSA record for the target sidecar | ||
| tlsarrset, err := tdns.LookupTLSA(s.Identity) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to lookup TLSA record: %v", err) | ||
| } | ||
|
|
||
| err = tdns.VerifyCertAgainstTLSA(tlsarrset, rawCert) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to verify certificate against TLSA record: %v", err) | ||
| // Use the TLSA record to authenticate the remote end securely | ||
| // (This is a simplified example, in a real implementation you would need to configure the TLS client with the TLSA record) | ||
| tlsConfig := &tls.Config{ | ||
| VerifyPeerCertificate: func(rawCerts [][]byte, verifiedChains [][]*x509.Certificate) error { | ||
| for _, rawCert := range rawCerts { | ||
| cert, err := x509.ParseCertificate(rawCert) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to parse certificate: %v", err) | ||
| } | ||
| if cert.Subject.CommonName != s.Identity { | ||
| return fmt.Errorf("unexpected certificate common name (should have been %s)", s.Identity) | ||
| } | ||
|
|
||
| err = tdns.VerifyCertAgainstTLSA(tlsarrset, rawCert) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to verify certificate against TLSA record: %v", err) | ||
| } | ||
| } | ||
| } | ||
| return nil | ||
| }, | ||
| } | ||
| return nil | ||
| }, | ||
| } | ||
|
|
||
| // Create the HTTPS client | ||
| client := &http.Client{ | ||
| Transport: &http.Transport{ | ||
| TLSClientConfig: tlsConfig, | ||
| }, | ||
| } | ||
| // Create the HTTPS client | ||
| client := &http.Client{ | ||
| Transport: &http.Transport{ | ||
| TLSClientConfig: tlsConfig, | ||
| }, | ||
| } | ||
|
|
||
| // Send the HTTPS POST request | ||
| url := fmt.Sprintf("https://%s:%d/hello", s.Identity, s.Port) | ||
| resp, err := client.Post(url, "application/json", bytes.NewBuffer(jsonData)) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to send HTTPS POST request: %v", err) | ||
| } | ||
| defer resp.Body.Close() | ||
| // Send the HTTPS POST request | ||
| url := fmt.Sprintf("https://%s:%d/hello", s.Identity, s.Port) | ||
| resp, err := client.Post(url, "application/json", bytes.NewBuffer(jsonData)) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to send HTTPS POST request: %v", err) | ||
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| // Print the response to stdout | ||
| body, err := io.ReadAll(resp.Body) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to read response body: %v", err) | ||
| } | ||
| fmt.Printf("Received response: %s\n", string(body)) |
There was a problem hiding this comment.
Update TLS Certificate Validation to Use SANs Instead of Common Name
The usage of cert.Subject.CommonName for certificate validation is deprecated and may not be reliable. It's recommended to validate the certificate using the Subject Alternative Name (SAN) extension.
Apply this diff to update the certificate validation:
- if cert.Subject.CommonName != s.Identity {
- return fmt.Errorf("unexpected certificate common name (should have been %s)", s.Identity)
- }
+ // Validate the certificate using SANs
+ found := false
+ for _, dnsName := range cert.DNSNames {
+ if dnsName == s.Identity {
+ found = true
+ break
+ }
+ }
+ if !found {
+ return fmt.Errorf("certificate does not contain expected SAN %s", s.Identity)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| switch s.Mechanism { | |
| case tdns.MSignMechanismAPI: | |
| // Create the SidecarHelloPost struct | |
| helloPost := SidecarHelloPost{ | |
| SidecarId: s.Identity, | |
| Addresses: s.Addresses, | |
| Port: s.Port, | |
| } | |
| // Lookup the TLSA record for the target sidecar | |
| tlsarrset, err := tdns.LookupTLSA(s.Identity) | |
| if err != nil { | |
| return fmt.Errorf("failed to lookup TLSA record: %v", err) | |
| } | |
| // Encode the struct as JSON | |
| jsonData, err := json.Marshal(helloPost) | |
| if err != nil { | |
| return fmt.Errorf("failed to marshal SidecarHelloPost: %v", err) | |
| } | |
| // Use the TLSA record to authenticate the remote end securely | |
| // (This is a simplified example, in a real implementation you would need to configure the TLS client with the TLSA record) | |
| tlsConfig := &tls.Config{ | |
| VerifyPeerCertificate: func(rawCerts [][]byte, verifiedChains [][]*x509.Certificate) error { | |
| for _, rawCert := range rawCerts { | |
| cert, err := x509.ParseCertificate(rawCert) | |
| if err != nil { | |
| return fmt.Errorf("failed to parse certificate: %v", err) | |
| } | |
| if cert.Subject.CommonName != s.Identity { | |
| return fmt.Errorf("unexpected certificate common name (should have been %s)", s.Identity) | |
| } | |
| // Lookup the TLSA record for the target sidecar | |
| tlsarrset, err := tdns.LookupTLSA(s.Identity) | |
| if err != nil { | |
| return fmt.Errorf("failed to lookup TLSA record: %v", err) | |
| } | |
| err = tdns.VerifyCertAgainstTLSA(tlsarrset, rawCert) | |
| if err != nil { | |
| return fmt.Errorf("failed to verify certificate against TLSA record: %v", err) | |
| // Use the TLSA record to authenticate the remote end securely | |
| // (This is a simplified example, in a real implementation you would need to configure the TLS client with the TLSA record) | |
| tlsConfig := &tls.Config{ | |
| VerifyPeerCertificate: func(rawCerts [][]byte, verifiedChains [][]*x509.Certificate) error { | |
| for _, rawCert := range rawCerts { | |
| cert, err := x509.ParseCertificate(rawCert) | |
| if err != nil { | |
| return fmt.Errorf("failed to parse certificate: %v", err) | |
| } | |
| if cert.Subject.CommonName != s.Identity { | |
| return fmt.Errorf("unexpected certificate common name (should have been %s)", s.Identity) | |
| } | |
| err = tdns.VerifyCertAgainstTLSA(tlsarrset, rawCert) | |
| if err != nil { | |
| return fmt.Errorf("failed to verify certificate against TLSA record: %v", err) | |
| } | |
| } | |
| } | |
| return nil | |
| }, | |
| } | |
| return nil | |
| }, | |
| } | |
| // Create the HTTPS client | |
| client := &http.Client{ | |
| Transport: &http.Transport{ | |
| TLSClientConfig: tlsConfig, | |
| }, | |
| } | |
| // Create the HTTPS client | |
| client := &http.Client{ | |
| Transport: &http.Transport{ | |
| TLSClientConfig: tlsConfig, | |
| }, | |
| } | |
| // Send the HTTPS POST request | |
| url := fmt.Sprintf("https://%s:%d/hello", s.Identity, s.Port) | |
| resp, err := client.Post(url, "application/json", bytes.NewBuffer(jsonData)) | |
| if err != nil { | |
| return fmt.Errorf("failed to send HTTPS POST request: %v", err) | |
| } | |
| defer resp.Body.Close() | |
| // Send the HTTPS POST request | |
| url := fmt.Sprintf("https://%s:%d/hello", s.Identity, s.Port) | |
| resp, err := client.Post(url, "application/json", bytes.NewBuffer(jsonData)) | |
| if err != nil { | |
| return fmt.Errorf("failed to send HTTPS POST request: %v", err) | |
| } | |
| defer resp.Body.Close() | |
| // Print the response to stdout | |
| body, err := io.ReadAll(resp.Body) | |
| if err != nil { | |
| return fmt.Errorf("failed to read response body: %v", err) | |
| } | |
| fmt.Printf("Received response: %s\n", string(body)) | |
| switch s.Mechanism { | |
| case tdns.MSignMechanismAPI: | |
| // Create the SidecarHelloPost struct | |
| helloPost := SidecarHelloPost{ | |
| SidecarId: s.Identity, | |
| Addresses: s.Addresses, | |
| Port: s.Port, | |
| } | |
| // Encode the struct as JSON | |
| jsonData, err := json.Marshal(helloPost) | |
| if err != nil { | |
| return fmt.Errorf("failed to marshal SidecarHelloPost: %v", err) | |
| } | |
| // Lookup the TLSA record for the target sidecar | |
| tlsarrset, err := tdns.LookupTLSA(s.Identity) | |
| if err != nil { | |
| return fmt.Errorf("failed to lookup TLSA record: %v", err) | |
| } | |
| // Use the TLSA record to authenticate the remote end securely | |
| // (This is a simplified example, in a real implementation you would need to configure the TLS client with the TLSA record) | |
| tlsConfig := &tls.Config{ | |
| VerifyPeerCertificate: func(rawCerts [][]byte, verifiedChains [][]*x509.Certificate) error { | |
| for _, rawCert := range rawCerts { | |
| cert, err := x509.ParseCertificate(rawCert) | |
| if err != nil { | |
| return fmt.Errorf("failed to parse certificate: %v", err) | |
| } | |
| // Validate the certificate using SANs | |
| found := false | |
| for _, dnsName := range cert.DNSNames { | |
| if dnsName == s.Identity { | |
| found = true | |
| break | |
| } | |
| } | |
| if !found { | |
| return fmt.Errorf("certificate does not contain expected SAN %s", s.Identity) | |
| } | |
| err = tdns.VerifyCertAgainstTLSA(tlsarrset, rawCert) | |
| if err != nil { | |
| return fmt.Errorf("failed to verify certificate against TLSA record: %v", err) | |
| } | |
| } | |
| return nil | |
| }, | |
| } | |
| // Create the HTTPS client | |
| client := &http.Client{ | |
| Transport: &http.Transport{ | |
| TLSClientConfig: tlsConfig, | |
| }, | |
| } | |
| // Send the HTTPS POST request | |
| url := fmt.Sprintf("https://%s:%d/hello", s.Identity, s.Port) | |
| resp, err := client.Post(url, "application/json", bytes.NewBuffer(jsonData)) | |
| if err != nil { | |
| return fmt.Errorf("failed to send HTTPS POST request: %v", err) | |
| } | |
| defer resp.Body.Close() | |
| // Print the response to stdout | |
| body, err := io.ReadAll(resp.Body) | |
| if err != nil { | |
| return fmt.Errorf("failed to read response body: %v", err) | |
| } | |
| fmt.Printf("Received response: %s\n", string(body)) |
| log.Printf("LoadSidecarConfig: Error: mconf.Sidecar.Api.Identity: %s viper: %v", mconf.Sidecar.Api.Identity, viper.GetString("sidecar.api.identity")) | ||
| dump.P(mconf.Sidecar) |
There was a problem hiding this comment.
Avoid logging sensitive configuration data
The use of dump.P(mconf.Sidecar) may output sensitive information, such as certificates or keys, to the logs. This could lead to security risks if logs are exposed.
Apply this diff to remove the sensitive logging:
- log.Printf("LoadSidecarConfig: Error: mconf.Sidecar.Api.Identity: %s viper: %v", mconf.Sidecar.Api.Identity, viper.GetString("sidecar.api.identity"))
- dump.P(mconf.Sidecar)
+ log.Printf("LoadSidecarConfig: Error: Sidecar API identity mismatch")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| log.Printf("LoadSidecarConfig: Error: mconf.Sidecar.Api.Identity: %s viper: %v", mconf.Sidecar.Api.Identity, viper.GetString("sidecar.api.identity")) | |
| dump.P(mconf.Sidecar) | |
| log.Printf("LoadSidecarConfig: Error: Sidecar API identity mismatch") |
| log.Printf("LoadMusicConfig: unmarshalling MUSIC config into struct") | ||
| err = viper.Unmarshal(&mconf) | ||
| if err != nil { | ||
| log.Fatalf("Error unmarshalling MUSIC config into struct: %v", err) | ||
| } |
There was a problem hiding this comment.
Handle potential errors without exposing sensitive information
While unmarshalling the config into the struct, log.Fatalf is used, which will terminate the program and may display sensitive information contained in err. Consider handling the error without exposing sensitive details.
Apply this diff to modify the error handling:
- log.Fatalf("Error unmarshalling MUSIC config into struct: %v", err)
+ log.Printf("Error unmarshalling MUSIC config into struct")
+ return fmt.Errorf("LoadMusicConfig: unable to unmarshal config")Committable suggestion skipped: line range outside the PR's diff.
| @@ -216,37 +250,21 @@ func (zd *ZoneData) DelegationSyncSetup(kdb *KeyDB) error { | |||
| } | |||
| resp, err := zd.KeyDB.Sig0KeyMgmt(nil, kp) | |||
| if err != nil { | |||
| return fmt.Errorf("DelegationSyncSetup(%s) failed to generate keypair: %v", zd.ZoneName, err) | |||
| return fmt.Errorf("Sig0KeyPreparation(%s) failed to generate keypair: %v", name, err) | |||
| } | |||
| zd.Logger.Printf(resp.Msg) | |||
There was a problem hiding this comment.
Fix non-constant format string in zd.Logger.Printf
Passing a non-constant string as the format string to Printf can lead to unexpected behavior or security vulnerabilities if the string contains formatting directives. It's safer to use Print or to specify a format string.
Apply this diff to fix the issue:
- zd.Logger.Printf(resp.Msg)
+ zd.Logger.Print(resp.Msg)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| zd.Logger.Printf(resp.Msg) | |
| zd.Logger.Print(resp.Msg) |
🧰 Tools
🪛 golangci-lint (1.61.0)
255-255: printf: non-constant format string in call to (*log.Logger).Printf
(govet)
| if err != nil { | ||
| log.Printf("DelegationSyncSetup: Zone %s: Error from BootstrapSig0KeyWithParent(): %v.", zd.ZoneName, err) | ||
| for _, tes := range ur.TargetStatus { | ||
| log.Printf("DelegationSyncSetup: Zone %s: TargetUpdateStatus: %v", zd.ZoneName, tes) | ||
| } |
There was a problem hiding this comment.
Handle potential nil ur in error handling
When BootstrapSig0KeyWithParent returns an error, it's possible that ur is nil. Accessing ur.TargetStatus without checking if ur is not nil could lead to a nil pointer dereference and runtime panic.
Apply this diff to fix the issue:
if err != nil {
log.Printf("DelegationSyncher: Zone %s: Error from BootstrapSig0KeyWithParent(): %v.", zd.ZoneName, err)
+ if ur != nil {
for _, tes := range ur.TargetStatus {
log.Printf("DelegationSyncher: Zone %s: TargetUpdateStatus: %v", zd.ZoneName, tes)
}
+ }
return err
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if err != nil { | |
| log.Printf("DelegationSyncSetup: Zone %s: Error from BootstrapSig0KeyWithParent(): %v.", zd.ZoneName, err) | |
| for _, tes := range ur.TargetStatus { | |
| log.Printf("DelegationSyncSetup: Zone %s: TargetUpdateStatus: %v", zd.ZoneName, tes) | |
| } | |
| if err != nil { | |
| log.Printf("DelegationSyncher: Zone %s: Error from BootstrapSig0KeyWithParent(): %v.", zd.ZoneName, err) | |
| if ur != nil { | |
| for _, tes := range ur.TargetStatus { | |
| log.Printf("DelegationSyncher: Zone %s: TargetUpdateStatus: %v", zd.ZoneName, tes) | |
| } | |
| } | |
| return err | |
| } |
sidecar auto zones.
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (14)
sidecar/apiserver.go (2)
117-124: Consider enhancing error handling.The early validation of configuration is good, but consider:
- Using structured errors instead of returning nil
- Adding more detailed error messages for debugging
Example improvement:
if len(addresses) == 0 { - log.Println("MusicSyncAPIdispatcher: no addresses to listen on. Not starting.") - return nil + return fmt.Errorf("MusicSyncAPIdispatcher: no addresses configured for listening") } if certFile == "" || keyFile == "" { - log.Println("MusicSyncAPIdispatcher: certFile or keyFile not set. Not starting.") - return nil + return fmt.Errorf("MusicSyncAPIdispatcher: missing required TLS configuration (certFile: %v, keyFile: %v)", + certFile != "", keyFile != "") }
130-130: Consider implementing graceful shutdown.The comment "unclear how to stop the http server nicely" indicates a known issue. Consider implementing a graceful shutdown mechanism using
http.Serverwith a context-based shutdown.Example approach:
srv := &http.Server{ Addr: addr, Handler: router, } go func() { <-done shutdownCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() if err := srv.Shutdown(shutdownCtx); err != nil { log.Printf("HTTP server shutdown error: %v", err) } }() if err := srv.ListenAndServeTLS(certFile, keyFile); err != http.ErrServerClosed { log.Printf("HTTP server error: %v", err) return err }Also applies to: 133-133
sidecar/main.go (4)
129-130: Consider removing or properly wrapping debug statementsThe commented debug statement
dump.P(mconf.Sidecar)should either be removed or properly wrapped in a debug condition using the existing debug flagtdns.Globals.Debug.- // dump.P(mconf.Sidecar) + if tdns.Globals.Debug { + dump.P(mconf.Sidecar) + }
Line range hint
170-174: Remove redundant error loggingThe error is printed twice using different methods. Since
log.Fatalfwill terminate the program after logging, thefmt.Printfstatement is redundant.err = music.LoadSidecarConfig(&mconf, &tconf, all_zones) if err != nil { - fmt.Printf("Error loading sidecar config: %v", err) log.Fatalf("Error loading sidecar config: %v", err) }
180-181: Add error handling for API dispatchersThe API dispatcher goroutines should have error handling mechanisms to report and handle failures. Consider using error channels or a WaitGroup with panic recovery.
+ errCh := make(chan error, 2) + go func() { + defer func() { + if r := recover(); r != nil { + errCh <- fmt.Errorf("APIdispatcher panic: %v", r) + } + }() go APIdispatcher(&tconf, &mconf, apistopper) + }() + go func() { + defer func() { + if r := recover(); r != nil { + errCh <- fmt.Errorf("MusicSyncAPIdispatcher panic: %v", r) + } + }() go MusicSyncAPIdispatcher(&tconf, &mconf, apistopper) + }() + + // Handle errors from API dispatchers + go func() { + for err := range errCh { + log.Printf("API dispatcher error: %v", err) + // Optionally trigger shutdown + close(apistopper) + } + }()
Line range hint
1-246: Consider implementing a structured component lifecycle managerThe file has grown complex with multiple goroutines and channels, making lifecycle management challenging. Consider implementing a component lifecycle manager to:
- Track and manage all goroutines
- Ensure consistent shutdown across all components
- Handle panic recovery and error propagation
- Provide health monitoring
This would improve maintainability and reliability of the application.
Would you like me to provide a detailed design proposal for this improvement?
tdns/sign.go (3)
Line range hint
192-266: Consider refactoring SignZone for better maintainabilityThe SignZone function handles multiple responsibilities and could benefit from being split into smaller, focused functions:
- Key management (promotion and generation)
- Zone signing
- NSEC chain management
This would improve:
- Testability
- Error handling
- Code maintainability
Consider extracting key management into a separate function:
+func (zd *ZoneData) ensureActiveKeys(kdb *KeyDB) (*DnssecKeys, error) { + dak, err := kdb.GetDnssecKeys(zd.ZoneName, DnskeyStateActive) + if err != nil { + return nil, fmt.Errorf("failed to get DNSSEC active keys: %w", err) + } + + if len(dak.KSKs) == 0 && len(dak.ZSKs) == 0 { + // Promote published keys if available + if err := zd.promotePublishedKeys(kdb); err != nil { + return nil, err + } + + // Generate new keys if needed + if err := zd.generateRequiredKeys(kdb); err != nil { + return nil, err + } + + // Get the final key set + return kdb.GetDnssecKeys(zd.ZoneName, DnskeyStateActive) + } + + return dak, nil +}
Line range hint
371-374: Remove commented out codeThe commented out code should be removed if it's no longer needed, or implemented if it represents a TODO item.
- // apex, err := zd.GetOwner(zd.ZoneName) - // if err != nil { - // return err - // }
Line range hint
192-266: Improve error handling with more contextThe error handling in SignZone could be enhanced by:
- Adding more context to errors using
fmt.Errorfwith%w- Using consistent error message formatting
- Including relevant state information in errors
Example improvement:
- return 0, fmt.Errorf("SignZone: failed to generate active keys for zone %s", zd.ZoneName) + return 0, fmt.Errorf("failed to generate active keys for zone %s: no KSKs available after generation attempt", zd.ZoneName)Consider wrapping errors consistently:
- return 0, err + return 0, fmt.Errorf("failed to get DNSSEC active keys for zone %s: %w", zd.ZoneName, err)tdns/parseconfig.go (1)
203-217: Consider separating database initialization logic.The database initialization logic could be moved to a dedicated package/module to better adhere to the Single Responsibility Principle. This would make the code more maintainable and testable.
Consider creating a separate
dbpackage with initialization logic, leaving only configuration-related code in this file.music/config.go (1)
183-190: Enhance error message with more specific guidanceWhile the validation logic is correct, the error message could be more helpful by including the configured DB mode.
- log.Printf("ValidateConfig: MUSIC DB file '%s' does not exist.", config.Db.File) - log.Printf("Please initialize MUSIC DB using 'sidecar-cli music db init -f %s'.", config.Db.File) + log.Printf("ValidateConfig: MUSIC DB file '%s' (mode: %s) does not exist.", config.Db.File, config.Db.Mode) + log.Printf("Please initialize MUSIC DB using 'sidecar-cli music db init -f %s -m %s'.", config.Db.File, config.Db.Mode)tdns/zone_utils.go (3)
798-802: LGTM! Consider enhancing the log message.The early return logic for zones not requiring delegation sync is well implemented.
Consider making the log message more descriptive:
- zd.Logger.Printf("SetupZoneSync: Zone %s does not require delegation sync", zd.ZoneName) + zd.Logger.Printf("SetupZoneSync: Zone %s: skipping delegation sync (neither parent nor child sync enabled)", zd.ZoneName)
889-893: LGTM! Consider making the timeout configurable.Good addition of timeout handling for the resign queue to prevent potential deadlocks. However, the 2-second timeout is hardcoded.
Consider making the timeout configurable:
+const defaultResignQueueTimeout = 2 * time.Second + select { case resignq <- zd: -case <-time.After(2 * time.Second): +case <-time.After(viper.GetDuration("dnsengine.resign.timeout") * time.Second): log.Printf("SetupZoneSigning: timeout while sending zone %s to resign queue", zd.ZoneName) }
1033-1043: Remove commented debug code.The commented-out debug code should be removed as it adds noise to the codebase.
Remove the entire commented block or convert it to proper debug logging if needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
music/config.go(6 hunks)sidecar/apiserver.go(2 hunks)sidecar/main.go(4 hunks)tdns/cli/keystore_cmds.go(4 hunks)tdns/parseconfig.go(4 hunks)tdns/sign.go(2 hunks)tdns/zone_utils.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tdns/cli/keystore_cmds.go
🔇 Additional comments (11)
sidecar/apiserver.go (5)
9-11: LGTM: Import additions are appropriate.
The new imports (fmt and net) are correctly added to support the enhanced address handling functionality.
109-110: Function rename improves clarity.
The rename from MusicAPIdispatcher to MusicSyncAPIdispatcher better reflects the function's purpose in handling sidecar-to-sidecar synchronization.
113-116: Improved configuration handling.
Good refactoring from viper.GetString/GetStringSlice to structured configuration. This change:
- Provides type safety
- Makes configuration dependencies explicit
- Improves maintainability
129-130: Enhanced address handling with proper port joining.
Good improvement using net.JoinHostPort for proper address:port combination. This handles IPv6 addresses correctly and is more robust than string concatenation.
109-109: Verify function rename impact.
The function has been renamed from MusicAPIdispatcher to MusicSyncAPIdispatcher. Let's verify all callers have been updated.
✅ Verification successful
Function rename has been properly propagated
All references to the function are using the new name MusicSyncAPIdispatcher. The function is:
- Defined in
sidecar/apiserver.go - Called in
sidecar/main.go - Referenced in log messages within its implementation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to the old function name
rg "MusicAPIdispatcher" --type go
# Search for references to the new function name to verify proper usage
rg "MusicSyncAPIdispatcher" --type go
Length of output: 620
sidecar/main.go (1)
187-188: LGTM: ResignQ channel creation is well-configured
The ResignQ channel is properly initialized with an appropriate buffer size for handling zone resigning operations.
tdns/sign.go (1)
235-235:
Consider security implications of policy dumps and use structured logging
The raw dump of DNSSEC policy could expose sensitive configuration information in logs. Consider:
- Using structured logging with specific fields instead of raw dumps
- Adding log level controls
- Sanitizing sensitive information before logging
Replace the raw dump with structured logging:
- dump.P(zd.DnssecPolicy)
+ log.Printf("SignZone: No active KSKs found for zone %s. Current policy: algorithm=%d",
+ zd.ZoneName, zd.DnssecPolicy.Algorithm)Let's verify if similar debug dumps exist elsewhere in the codebase:
✅ Verification successful
Let me gather more context about the usage of dump.P in the codebase to understand the logging patterns better.
Based on the analysis of the codebase, I can now generate the final response:
Consider using structured logging for DNSSEC policy information
The codebase already extensively uses structured logging via log.Printf() with specific fields and context. The raw dump of DNSSEC policy should follow the same pattern:
- dump.P(zd.DnssecPolicy)
+ log.Printf("SignZone: No active KSKs found for zone %s. DNSSEC policy: algorithm=%d, ksk_keysize=%d, zsk_keysize=%d",
+ zd.ZoneName, zd.DnssecPolicy.Algorithm, zd.DnssecPolicy.KskKeysize, zd.DnssecPolicy.ZskKeysize)Rationale:
- The codebase consistently uses structured logging via
log.Printf()for sensitive operations - The dump is only used in debug scenarios when no active KSKs are found
- The suggested change maintains debug information while following established logging patterns
- Other instances of
dump.P()in the codebase are either commented out or used for non-sensitive data
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other instances of raw policy dumps
rg "dump\.P.*Policy"
# Search for other uses of the dump package
rg "goutil/dump"
Length of output: 886
Script:
#!/bin/bash
# Search for actual dump.P usage patterns
rg "dump\.P\(" -A 2
# Check if structured logging is already used in the codebase
rg "log\.(Print|Info|Debug|Error|Warn)"
Length of output: 122837
tdns/parseconfig.go (2)
9-9: LGTM: Good addition of error handling.
The addition of the errors package import supports the improved error handling throughout the code.
206-214:
Security concern: Database file path validation still needed.
While the existence check is good, the database file path should be validated to prevent directory traversal attacks, as mentioned in a previous review.
music/config.go (2)
41-58: LGTM! Well-structured configuration separation
The separation of API and DNS configurations into distinct structures with shared common fields improves maintainability and clarity.
277-283: Previous review comment about sensitive data logging is still applicable
The error handling should be improved to avoid exposing sensitive configuration data in logs.
| // The ResignerEngine is needed only for the sidecar auto zones. | ||
| go tdns.ResignerEngine(tconf.Internal.ResignQ, make(chan struct{})) |
There was a problem hiding this comment.
Use the existing stop channel for ResignerEngine
Creating an anonymous stop channel (make(chan struct{})) makes it impossible to gracefully stop the ResignerEngine goroutine. Consider using the existing stopch for consistent shutdown management.
- go tdns.ResignerEngine(tconf.Internal.ResignQ, make(chan struct{}))
+ go tdns.ResignerEngine(tconf.Internal.ResignQ, stopch)
music/config.go
Outdated
| } | ||
|
|
||
| log.Printf("LoadSidecarConfig: Successfully published TLSA RR for sidecar identity '%s'\n", mconf.Sidecar.Identity) | ||
| ur := tdns.UpdateRequest{ | ||
| Cmd: "DEFERRED-UPDATE", | ||
| ZoneName: mconf.Sidecar.Dns.Identity, | ||
| Description: fmt.Sprintf("Publish SVCB + SIG(0) KEY RR for sidecar DNS identity '%s'", mconf.Sidecar.Dns.Identity), | ||
| PreCondition: func() bool { | ||
| _, ok := tdns.Zones.Get(mconf.Sidecar.Dns.Identity) | ||
| if !ok { | ||
| log.Printf("LoadSidecarConfig: zone data for sidecar identity '%s' still not found", mconf.Sidecar.Dns.Identity) | ||
| } | ||
| return ok | ||
| }, | ||
| Action: func() error { | ||
| var err error | ||
| zd, ok := tdns.Zones.Get(mconf.Sidecar.Dns.Identity) | ||
| if !ok { | ||
| return fmt.Errorf("LoadSidecarConfig: Action: zone data for sidecar identity '%s' still not found", mconf.Sidecar.Dns.Identity) | ||
| } | ||
| zd.Options[tdns.OptAllowUpdates] = true | ||
|
|
||
| log.Printf("LoadSidecarConfig: sending PING command to sidecar '%s'", mconf.Sidecar.Dns.Identity) | ||
| zd.KeyDB.UpdateQ <- tdns.UpdateRequest{ | ||
| Cmd: "PING", | ||
| } | ||
|
|
||
| log.Printf("LoadSidecarConfig: publishing KEY RR for sidecar identity '%s' SIG(0) public key", mconf.Sidecar.Dns.Identity) | ||
|
|
||
| err = zd.MusicSig0KeyPrep(mconf.Sidecar.Dns.Identity, zd.KeyDB) | ||
| if err != nil { | ||
| return fmt.Errorf("LoadSidecarConfig: failed to publish KEY RR for sidecar DNS identity '%s' SIG(0) public key: %v", mconf.Sidecar.Dns.Identity, err) | ||
| } | ||
|
|
||
| log.Printf("LoadSidecarConfig: Successfully published KEY RR for sidecar DNS identity '%s' SIG(0) public key\n", mconf.Sidecar.Dns.Identity) | ||
|
|
||
| var ipv4hint, ipv6hint []net.IP | ||
| if len(mconf.Sidecar.Dns.Addresses) > 0 { | ||
| for _, addr := range mconf.Sidecar.Dns.Addresses { | ||
| ip := net.ParseIP(addr) | ||
| if ip.To4() != nil { | ||
| ipv4hint = append(ipv4hint, ip) | ||
| } else { | ||
| ipv6hint = append(ipv6hint, ip) | ||
| } | ||
| } | ||
| } | ||
| var value []dns.SVCBKeyValue | ||
|
|
||
| if mconf.Sidecar.Dns.Port != 0 { | ||
| value = append(value, &dns.SVCBPort{Port: mconf.Sidecar.Dns.Port}) | ||
| } | ||
|
|
||
| if len(ipv4hint) > 0 { | ||
| value = append(value, &dns.SVCBIPv4Hint{Hint: ipv4hint}) | ||
| } | ||
|
|
||
| if len(ipv6hint) > 0 { | ||
| value = append(value, &dns.SVCBIPv6Hint{Hint: ipv6hint}) | ||
| } | ||
|
|
||
| log.Printf("LoadSidecarConfig: publishing SVCB RR for sidecar DNS identity '%s'", mconf.Sidecar.Dns.Identity) | ||
| err = zd.PublishSvcbRR(mconf.Sidecar.Dns.Identity, mconf.Sidecar.Dns.Port, value) | ||
| if err != nil { | ||
| return fmt.Errorf("LoadSidecarConfig: failed to publish SVCB RR: %v", err) | ||
| } | ||
| log.Printf("LoadSidecarConfig: Successfully published SVCB RR for sidecar identity '%s'\n", mconf.Sidecar.Dns.Identity) | ||
|
|
||
| for _, addr := range mconf.Sidecar.Dns.Addresses { | ||
| err = zd.PublishAddrRR(mconf.Sidecar.Dns.Identity, addr) | ||
| if err != nil { | ||
| return fmt.Errorf("LoadSidecarConfig: failed to publish address RR: %v", err) | ||
| } | ||
| } | ||
| log.Printf("LoadSidecarConfig: Successfully published address RRs for sidecar DNS identity '%s'\n", mconf.Sidecar.Dns.Identity) | ||
|
|
||
| return nil | ||
| }, | ||
| } | ||
|
|
||
| apex, _ := zd.Data.Get(zd.ZoneName) | ||
| if err != nil { | ||
| return fmt.Errorf("LoadSidecarConfig: Error: failed to get zone apex %s: %v", zd.ZoneName, err) | ||
| } | ||
| mconf.Internal.UpdateQ <- ur | ||
| } | ||
|
|
||
| tlsarr_rrset := apex.RRtypes.GetOnlyRRSet(dns.TypeTLSA) | ||
| var tlsarr *dns.TLSA | ||
| if len(tlsarr_rrset.RRs) > 0 { | ||
| tlsarr = tlsarr_rrset.RRs[0].(*dns.TLSA) | ||
| log.Printf("LoadSidecarConfig: TLSA RR: %s", tlsarr.String()) | ||
| } else { | ||
| log.Printf("LoadSidecarConfig: Error: TLSA: %v", tlsarr_rrset) | ||
| return fmt.Errorf("LoadSidecarConfig: Error: TLSA: %v", tlsarr_rrset) | ||
| } | ||
| return nil | ||
| }, | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider breaking down the large function into smaller, focused functions
The LoadSidecarConfig function is handling multiple responsibilities including certificate validation, DNS record publishing, and configuration validation. Consider splitting it into smaller functions for better maintainability:
validateSidecarApiConfigvalidateSidecarDnsConfigpublishSidecarApiRecordspublishSidecarDnsRecords
Additionally, consider implementing retry logic for DNS record publishing operations, as they might fail due to temporary network issues.
music/config.go
Outdated
| if tmp, exists := tconf.Internal.DnssecPolicies["default"]; !exists { | ||
| log.Fatalf("SetupSidecarAutoZone: DnssecPolicy 'default' not defined. Default policy is required for sidecar auto zones.") | ||
| } else { | ||
| zd.DnssecPolicy = &tmp | ||
| } | ||
|
|
There was a problem hiding this comment.
Replace fatal error with returnable error
Using log.Fatalf prevents graceful error handling and recovery. Consider returning an error instead.
- log.Fatalf("SetupSidecarAutoZone: DnssecPolicy 'default' not defined. Default policy is required for sidecar auto zones.")
+ return nil, fmt.Errorf("SetupSidecarAutoZone: DnssecPolicy 'default' not defined. Default policy is required for sidecar auto zones")Committable suggestion skipped: line range outside the PR's diff.
music/config.go
Outdated
| certPEM, err := os.ReadFile(mconf.Sidecar.Api.Cert) | ||
| if err != nil { | ||
| return fmt.Errorf("LoadSidecarConfig: error reading cert file: %v", err) | ||
| } | ||
|
|
||
| mconf.Sidecar.Cert = string(certPEM) | ||
| mconf.Sidecar.Key = string(keyPEM) | ||
| keyPEM, err := os.ReadFile(mconf.Sidecar.Api.Key) | ||
| if err != nil { | ||
| return fmt.Errorf("LoadSidecarConfig: error reading key file: %v", err) | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Based on the gathered information, I can now provide the final response:
Security improvements needed for certificate handling
The code requires additional security measures for certificate and key handling:
- No file permission checks are performed before reading certificate/key files
- The
SidecarApiConfstruct allows both file paths and raw data (CertData/KeyData), but the code only implements the file-based approach without proper validation - While
filepath.Cleanis used in some parts of the codebase, it's not used for certificate/key paths
Required changes:
- Add permission checks to ensure certificate/key files are not world-readable
- Implement path sanitization using
filepath.Cleanfor certificate/key paths - Consider using the existing
CertData/KeyDatafields instead of files for secure in-memory handling
🔗 Analysis chain
Verify secure handling of certificate and key files
The code reads certificate and key files directly. Ensure:
- File permissions are properly set
- Key material is securely handled in memory
- File paths are validated to prevent path traversal
Let me gather more information about file permission checks and path validation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potentially insecure file operations
rg -A 2 "os\.Open|os\.Read|ioutil\.Read"
# Check for proper certificate/key file permission validation
ast-grep --pattern 'os.Stat($path) {
$$$
FileMode()
$$$
}'
Length of output: 5368
Script:
#!/bin/bash
# Check for file permission validation
rg -A 3 "os\.Chmod|os\.Stat|FileMode|[^a-zA-Z]mode.*=.*0[0-9]{3}"
# Check for path validation and sanitization
rg -A 2 "filepath\.Clean|filepath\.Abs|filepath\.IsAbs|filepath\.Rel"
# Look for configuration struct to understand how cert/key paths are defined
ast-grep --pattern 'type $_ struct {
$$$
Cert string
Key string
$$$
}'
Length of output: 4109
* bugfix in the key generator * various debug tweaks
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (7)
tdns/sig0_utils.go (2)
159-166: Consider enhancing logging with additional key metadata.While the current logging is helpful, consider including additional metadata such as the algorithm and key ID for better traceability.
- log.Printf("Generated KEY flags: %d", nkey.(*dns.KEY).Flags) + log.Printf("Generated KEY: flags=%d algorithm=%d keyid=%d", nkey.(*dns.KEY).Flags, nkey.(*dns.KEY).Algorithm, nkey.(*dns.KEY).KeyTag()) - log.Printf("Generated DNSKEY flags: %d", nkey.(*dns.DNSKEY).Flags) + log.Printf("Generated DNSKEY: flags=%d algorithm=%d keyid=%d", nkey.(*dns.DNSKEY).Flags, nkey.(*dns.DNSKEY).Algorithm, nkey.(*dns.DNSKEY).KeyTag())
Line range hint
20-23: TODO/FIXME comments need attention.There are two FIXME comments indicating that:
- The code is only used from CLI and should be changed to be used by TDNS-SERVER via API
- The code is not ready to generate DNSSEC keys due to flags field concerns
Would you like me to help create GitHub issues to track these TODOs or assist in implementing the required changes?
music/config.go (3)
189-196: Consider adding more context to error messagesThe DB file validation logic is good, but the error messages could be more informative.
- return errors.New("ValidateConfig: MUSIC DB file does not exist") + return fmt.Errorf("ValidateConfig: MUSIC DB file does not exist at path '%s'", config.Db.File)
565-594: Consider adding retry mechanism for zone operationsThe
SetupSidecarAutoZonefunction performs several critical operations (zone creation, signing setup, sync setup) but doesn't include any retry mechanism for transient failures.Consider implementing a retry mechanism with backoff for operations like:
- Zone creation
- Setting up zone signing
- Setting up delegation syncing
381-458: Consider breaking down the large anonymous functionThe anonymous function in
UpdateRequest.Actionis quite large and handles multiple responsibilities. Consider breaking it down into smaller, focused functions for better maintainability.Consider extracting these operations into separate functions:
publishTLSAandSVCBsetupIPHintspublishAddressRecordstdns/zone_utils.go (2)
889-893: Consider making the timeout duration configurable.While the timeout handling is a good addition to prevent deadlocks, the 2-second duration could be made configurable through viper config to allow adjustment based on system requirements.
-case <-time.After(2 * time.Second): +case <-time.After(time.Duration(viper.GetInt("zone.resign.timeout")) * time.Second):
1033-1043: Remove or uncomment debug logging code.The commented-out debug logging code should either be removed or uncommented if it serves a purpose.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
music/config.go(7 hunks)music/defaults.go(1 hunks)sidecar/apiserver.go(2 hunks)sidecar/main.go(3 hunks)tdns/parseconfig.go(7 hunks)tdns/sig0_utils.go(1 hunks)tdns/zone_utils.go(4 hunks)utils/Makefile.common(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- music/defaults.go
🚧 Files skipped from review as they are similar to previous changes (3)
- sidecar/apiserver.go
- sidecar/main.go
- utils/Makefile.common
🔇 Additional comments (12)
tdns/sig0_utils.go (2)
161-161: LGTM! Helpful debug logging for key generation.
The added logging statements are valuable for debugging and auditing key generation, making it easier to verify the correct flags are set for both KEY and DNSKEY records.
Also applies to: 164-164
159-166: Verify key generation flags across the codebase.
Let's ensure the key flags are used consistently across the codebase.
✅ Verification successful
Let me gather more context about the key types and their usage patterns.
Based on the comprehensive search results, I can now provide a final response about the key flag verification:
Key flags are being used consistently across the codebase
The key flag usage follows the standard DNSSEC practices:
- ZSK (Zone Signing Key) consistently uses flag value 256
- KSK (Key Signing Key) and CSK (Combined Signing Key) consistently use flag value 257
- The flag values are properly checked and validated throughout the codebase, particularly in key management and validation functions
The implementation in sig0_utils.go correctly handles:
- Default flag setting of 256 for both KEY and DNSKEY types
- Proper elevation to 257 for KSK/CSK types
- Consistent logging of flag values during key generation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for key flag usage patterns across the codebase
# Search for KEY/DNSKEY flag assignments
echo "Searching for KEY/DNSKEY flag assignments..."
rg -A 2 "(?:KEY|DNSKEY).*Flags\s*=\s*\d+"
# Search for potential inconsistencies in flag values
echo "Searching for potential hardcoded flag values..."
rg "\bFlags\s*=\s*(256|257|0x100|0x101)\b"
Length of output: 846
Script:
#!/bin/bash
# Search for key type definitions and constants
echo "Searching for key type definitions..."
rg -A 2 "(?i)(?:ZSK|KSK|CSK)"
# Search for any documentation or comments about key flags
echo "Searching for key flag documentation..."
rg -B 2 -A 2 "(?i)(?:key.*flags?|flags?.*key)"
# Search for any test cases involving key flags
echo "Searching for key flag test cases..."
rg -A 3 "(?:KEY|DNSKEY).*Test"
Length of output: 38773
tdns/parseconfig.go (5)
73-75: LGTM: Debug logging properly implemented.
The debug logging is correctly guarded by the Debug flag and provides clear entry point indication.
126-129: Use consistent error handling instead of log.Fatalf.
The error handling here should follow the same pattern as other parts of the code by returning an error instead of calling log.Fatalf.
236-239: LGTM: Debug logging properly implemented.
The debug logging is correctly guarded by the Debug flag and provides clear exit point indication.
245-247: LGTM: Debug logging properly implemented.
The debug logging is correctly guarded by the Debug flag and provides clear entry point indication.
208-218:
Add path validation for database file.
While the existence check is good, the database file path should be validated to prevent directory traversal attacks.
Apply this diff to add path validation:
dbFile := viper.GetString("db.file")
+// Ensure the database file path is within allowed boundaries
+dbFile = filepath.Clean(dbFile)
+if strings.Contains(dbFile, "..") {
+ return errors.New("ParseConfig: invalid database file path: must not contain directory traversal")
+}
if conf.AppName != "sidecar-cli" && conf.AppName != "tdns-cli" {Likely invalid or redundant comment.
music/config.go (3)
41-64: LGTM: Well-structured type definitions
The separation of SidecarConf into SidecarApiConf and SidecarDnsConf improves modularity and clarity. The structure properly encapsulates related fields.
286-290:
Previous critical issue still applies
The use of log.Fatalf prevents proper error handling and may expose sensitive information.
374-375:
Previous security concern still applies
The logging of sensitive configuration data remains a security risk.
tdns/zone_utils.go (2)
797-802: LGTM! Channel direction specification and early return optimization.
The changes improve type safety by specifying the channel direction and optimize performance by adding an early return when sync is not required.
997-1011: 🛠️ Refactor suggestion
Make zone template configuration more flexible.
The zone template uses hardcoded values that should be configurable:
- TTL values
- SOA parameters
- NS name pattern
- Default IP address
+type ZoneTemplate struct {
+ TTL int
+ RefreshTime int
+ RetryTime int
+ ExpireTime int
+ MinimumTime int
+ DefaultIP string
+}
+
+func getDefaultTemplate() ZoneTemplate {
+ return ZoneTemplate{
+ TTL: viper.GetInt("zone.default.ttl") || 86400,
+ RefreshTime: viper.GetInt("zone.default.refresh") || 3600,
+ RetryTime: viper.GetInt("zone.default.retry") || 1800,
+ ExpireTime: viper.GetInt("zone.default.expire") || 1209600,
+ MinimumTime: viper.GetInt("zone.default.minimum") || 86400,
+ DefaultIP: viper.GetString("zone.default.ns_ip") || "192.0.2.1",
+ }
+}Likely invalid or redundant comment.
| ur := tdns.UpdateRequest{ | ||
| Cmd: "DEFERRED-UPDATE", | ||
| ZoneName: mconf.Sidecar.Dns.Identity, | ||
| Description: fmt.Sprintf("Publish SVCB + SIG(0) KEY RR for sidecar DNS identity '%s'", mconf.Sidecar.Dns.Identity), | ||
| PreCondition: func() bool { | ||
| _, ok := tdns.Zones.Get(mconf.Sidecar.Dns.Identity) | ||
| if !ok { | ||
| log.Printf("LoadSidecarConfig: zone data for sidecar identity '%s' still not found", mconf.Sidecar.Dns.Identity) | ||
| } | ||
| return ok | ||
| }, | ||
| Action: func() error { | ||
| var err error | ||
| zd, ok := tdns.Zones.Get(mconf.Sidecar.Dns.Identity) | ||
| if !ok { | ||
| return fmt.Errorf("LoadSidecarConfig: Action: zone data for sidecar identity '%s' still not found", mconf.Sidecar.Dns.Identity) | ||
| } | ||
| zd.Options[tdns.OptAllowUpdates] = true | ||
|
|
||
| log.Printf("LoadSidecarConfig: sending PING command to sidecar '%s'", mconf.Sidecar.Dns.Identity) | ||
| zd.KeyDB.UpdateQ <- tdns.UpdateRequest{ | ||
| Cmd: "PING", | ||
| } | ||
|
|
||
| log.Printf("LoadSidecarConfig: publishing KEY RR for sidecar identity '%s' SIG(0) public key", mconf.Sidecar.Dns.Identity) | ||
|
|
||
| err = zd.MusicSig0KeyPrep(mconf.Sidecar.Dns.Identity, zd.KeyDB) | ||
| if err != nil { | ||
| return fmt.Errorf("LoadSidecarConfig: failed to publish KEY RR for sidecar DNS identity '%s' SIG(0) public key: %v", mconf.Sidecar.Dns.Identity, err) | ||
| } | ||
|
|
||
| log.Printf("LoadSidecarConfig: Successfully published KEY RR for sidecar DNS identity '%s' SIG(0) public key\n", mconf.Sidecar.Dns.Identity) | ||
|
|
||
| var ipv4hint, ipv6hint []net.IP | ||
| if len(mconf.Sidecar.Dns.Addresses.Publish) > 0 { | ||
| for _, addr := range mconf.Sidecar.Dns.Addresses.Publish { | ||
| ip := net.ParseIP(addr) | ||
| if ip == nil { | ||
| log.Printf("LoadSidecarConfig: failed to parse address '%s'", addr) | ||
| continue | ||
| } | ||
| if ip.To4() != nil { | ||
| ipv4hint = append(ipv4hint, ip) | ||
| } else { | ||
| ipv6hint = append(ipv6hint, ip) | ||
| } | ||
| } | ||
| } | ||
| var value []dns.SVCBKeyValue | ||
|
|
||
| if mconf.Sidecar.Dns.Port != 0 { | ||
| value = append(value, &dns.SVCBPort{Port: mconf.Sidecar.Dns.Port}) | ||
| } | ||
|
|
||
| if len(ipv4hint) > 0 { | ||
| value = append(value, &dns.SVCBIPv4Hint{Hint: ipv4hint}) | ||
| } | ||
|
|
||
| if len(ipv6hint) > 0 { | ||
| value = append(value, &dns.SVCBIPv6Hint{Hint: ipv6hint}) | ||
| } | ||
|
|
||
| log.Printf("LoadSidecarConfig: publishing SVCB RR for sidecar DNS identity '%s'", mconf.Sidecar.Dns.Identity) | ||
| err = zd.PublishSvcbRR(mconf.Sidecar.Dns.Identity, mconf.Sidecar.Dns.Port, value) | ||
| if err != nil { | ||
| return fmt.Errorf("LoadSidecarConfig: failed to publish SVCB RR: %v", err) | ||
| } | ||
| log.Printf("LoadSidecarConfig: Successfully published SVCB RR for sidecar identity '%s'\n", mconf.Sidecar.Dns.Identity) | ||
|
|
||
| for _, addr := range mconf.Sidecar.Dns.Addresses.Publish { | ||
| err = zd.PublishAddrRR(mconf.Sidecar.Dns.Identity, addr) | ||
| if err != nil { | ||
| return fmt.Errorf("LoadSidecarConfig: failed to publish address RR: %v", err) | ||
| } | ||
| } | ||
| log.Printf("LoadSidecarConfig: Successfully published address RRs for sidecar DNS identity '%s'\n", mconf.Sidecar.Dns.Identity) | ||
|
|
||
| return nil | ||
| }, | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reduce code duplication with DNS identity setup
The DNS identity setup code largely duplicates the API identity setup logic.
Consider extracting common functionality into shared helper functions:
setupIPHintsfor IP address parsing and categorizationpublishServiceRecordsfor SVCB record publishingpublishAddressRecordsfor address record publishing
| if tmp, exists := tconf.Internal.DnssecPolicies["default"]; !exists { | ||
| log.Fatalf("SetupSidecarAutoZone: DnssecPolicy 'default' not defined. Default policy is required for sidecar auto zones.") | ||
| } else { |
There was a problem hiding this comment.
Replace fatal error with returnable error
Using log.Fatalf prevents proper error handling and recovery.
- log.Fatalf("SetupSidecarAutoZone: DnssecPolicy 'default' not defined. Default policy is required for sidecar auto zones.")
+ return nil, fmt.Errorf("SetupSidecarAutoZone: DnssecPolicy 'default' not defined. Default policy is required for sidecar auto zones")Committable suggestion skipped: line range outside the PR's diff.
| certPEM, err := os.ReadFile(mconf.Sidecar.Api.Cert) | ||
| if err != nil { | ||
| return fmt.Errorf("LoadSidecarConfig: error reading cert file: %v", err) | ||
| } | ||
|
|
||
| block, _ := pem.Decode(certPEM) | ||
| if block == nil { | ||
| return fmt.Errorf("LoadSidecarConfig: failed to parse certificate PEM") | ||
| } | ||
| keyPEM, err := os.ReadFile(mconf.Sidecar.Api.Key) | ||
| if err != nil { | ||
| return fmt.Errorf("LoadSidecarConfig: error reading key file: %v", err) | ||
| } |
There was a problem hiding this comment.
Add file permission checks for certificate and key files
The code reads sensitive files without verifying their permissions.
+ // Check file permissions
+ certInfo, err := os.Stat(mconf.Sidecar.Api.Cert)
+ if err != nil {
+ return fmt.Errorf("LoadSidecarConfig: error checking cert file permissions: %v", err)
+ }
+ if certInfo.Mode().Perm()&0077 != 0 {
+ return fmt.Errorf("LoadSidecarConfig: cert file has unsafe permissions: %v", certInfo.Mode().Perm())
+ }
+
+ keyInfo, err := os.Stat(mconf.Sidecar.Api.Key)
+ if err != nil {
+ return fmt.Errorf("LoadSidecarConfig: error checking key file permissions: %v", err)
+ }
+ if keyInfo.Mode().Perm()&0077 != 0 {
+ return fmt.Errorf("LoadSidecarConfig: key file has unsafe permissions: %v", keyInfo.Mode().Perm())
+ }
+
certPEM, err := os.ReadFile(mconf.Sidecar.Api.Cert)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| certPEM, err := os.ReadFile(mconf.Sidecar.Api.Cert) | |
| if err != nil { | |
| return fmt.Errorf("LoadSidecarConfig: error reading cert file: %v", err) | |
| } | |
| block, _ := pem.Decode(certPEM) | |
| if block == nil { | |
| return fmt.Errorf("LoadSidecarConfig: failed to parse certificate PEM") | |
| } | |
| keyPEM, err := os.ReadFile(mconf.Sidecar.Api.Key) | |
| if err != nil { | |
| return fmt.Errorf("LoadSidecarConfig: error reading key file: %v", err) | |
| } | |
| // Check file permissions | |
| certInfo, err := os.Stat(mconf.Sidecar.Api.Cert) | |
| if err != nil { | |
| return fmt.Errorf("LoadSidecarConfig: error checking cert file permissions: %v", err) | |
| } | |
| if certInfo.Mode().Perm()&0077 != 0 { | |
| return fmt.Errorf("LoadSidecarConfig: cert file has unsafe permissions: %v", certInfo.Mode().Perm()) | |
| } | |
| keyInfo, err := os.Stat(mconf.Sidecar.Api.Key) | |
| if err != nil { | |
| return fmt.Errorf("LoadSidecarConfig: error checking key file permissions: %v", err) | |
| } | |
| if keyInfo.Mode().Perm()&0077 != 0 { | |
| return fmt.Errorf("LoadSidecarConfig: key file has unsafe permissions: %v", keyInfo.Mode().Perm()) | |
| } | |
| certPEM, err := os.ReadFile(mconf.Sidecar.Api.Cert) | |
| if err != nil { | |
| return fmt.Errorf("LoadSidecarConfig: error reading cert file: %v", err) | |
| } | |
| keyPEM, err := os.ReadFile(mconf.Sidecar.Api.Key) | |
| if err != nil { | |
| return fmt.Errorf("LoadSidecarConfig: error reading key file: %v", err) | |
| } |
Summary by CodeRabbit
Release Notes
New Features
cleantarget in multiple Makefiles for easier cleanup of compiled files and temporary files across various components.Bug Fixes
Documentation
Refactor
Chores
go.modfile to reflect the new project structure.