permcheck: imp code, docs

internal/permcheck/check_unix.go

@@ -25,71 +25,64 @@ func check(
 	querylogDir string,
 	confFilePath string,
 ) {
-	checkDir(ctx, l, workDir)
+	dirLoggger, fileLogger := l.With("type", typeDir), l.With("type", typeFile)
 
-	checkFile(ctx, l, confFilePath)
+	checkDir(ctx, dirLoggger, workDir)
+
+	checkFile(ctx, fileLogger, confFilePath)
 
 	// TODO(a.garipov): Put all paths in one place and remove this duplication.
-	checkDir(ctx, l, dataDir)
-	checkDir(ctx, l, filepath.Join(dataDir, "filters"))
-	checkFile(ctx, l, filepath.Join(dataDir, "sessions.db"))
-	checkFile(ctx, l, filepath.Join(dataDir, "leases.json"))
+	checkDir(ctx, dirLoggger, dataDir)
+	checkDir(ctx, dirLoggger, filepath.Join(dataDir, "filters"))
+	checkFile(ctx, fileLogger, filepath.Join(dataDir, "sessions.db"))
+	checkFile(ctx, fileLogger, filepath.Join(dataDir, "leases.json"))
 
 	if dataDir != querylogDir {
-		checkDir(ctx, l, querylogDir)
+		checkDir(ctx, dirLoggger, querylogDir)
 	}
-	checkFile(ctx, l, filepath.Join(querylogDir, "querylog.json"))
-	checkFile(ctx, l, filepath.Join(querylogDir, "querylog.json.1"))
+	checkFile(ctx, fileLogger, filepath.Join(querylogDir, "querylog.json"))
+	checkFile(ctx, fileLogger, filepath.Join(querylogDir, "querylog.json.1"))
 
 	if dataDir != statsDir {
-		checkDir(ctx, l, statsDir)
+		checkDir(ctx, dirLoggger, statsDir)
 	}
-	checkFile(ctx, l, filepath.Join(statsDir, "stats.db"))
+	checkFile(ctx, fileLogger, filepath.Join(statsDir, "stats.db"))
 }
 
 // checkDir checks the permissions of a single directory.  The results are
 // logged at the appropriate level.
 func checkDir(ctx context.Context, l *slog.Logger, dirPath string) {
-	checkPath(ctx, l, dirPath, typeDir, aghos.DefaultPermDir)
+	checkPath(ctx, l, dirPath, aghos.DefaultPermDir)
 }
 
 // checkFile checks the permissions of a single file.  The results are logged at
 // the appropriate level.
 func checkFile(ctx context.Context, l *slog.Logger, filePath string) {
-	checkPath(ctx, l, filePath, typeFile, aghos.DefaultPermFile)
+	checkPath(ctx, l, filePath, aghos.DefaultPermFile)
 }
 
 // checkPath checks the permissions of a single filesystem entity.  The results
 // are logged at the appropriate level.
-func checkPath(ctx context.Context, l *slog.Logger, entPath, fileType string, want fs.FileMode) {
-	s, err := os.Stat(entPath)
+func checkPath(ctx context.Context, l *slog.Logger, fpath string, want fs.FileMode) {
+	l = l.With("path", fpath)
+	s, err := os.Stat(fpath)
 	if err != nil {
-		logFunc := l.ErrorContext
+		lvl := slog.LevelError
 		if errors.Is(err, os.ErrNotExist) {
-			logFunc = l.DebugContext
+			lvl = slog.LevelDebug
 		}
 
-		logFunc(
-			ctx,
-			"checking permissions",
-			"type", fileType,
-			"path", entPath,
-			slogutil.KeyError, err,
-		)
+		l.Log(ctx, lvl, "checking permissions", slogutil.KeyError, err)
 
 		return
 	}
 
 	// TODO(a.garipov): Add a more fine-grained check and result reporting.
 	perm := s.Mode().Perm()
-	if perm != want {
-		l.WarnContext(
-			ctx,
-			"found unexpected permissions",
-			"type", fileType,
-			"path", entPath,
-			"got", fmt.Sprintf("%#o", perm),
-			"want", fmt.Sprintf("%#o", want),
-		)
+	if perm == want {
+		return
 	}
+
+	permOct, wantOct := fmt.Sprintf("%#o", perm), fmt.Sprintf("%#o", want)
+	l.WarnContext(ctx, "found unexpected permissions", "perm", permOct, "want", wantOct)
 }

internal/permcheck/check_windows.go

@@ -12,6 +12,8 @@ import (
 
 // check is the Windows-specific implementation of [Check].
 func check(ctx context.Context, l *slog.Logger, workDir, _, _, _, _ string) {
+	l = l.With("type", typeDir, "path", workDir)
+
 	dacl, owner, err := getSecurityInfo(workDir)
 	if err != nil {
 		l.ErrorContext(ctx, "getting security info", slogutil.KeyError, err)
@@ -20,12 +22,14 @@ func check(ctx context.Context, l *slog.Logger, workDir, _, _, _, _ string) {
 	}
 
 	if !owner.IsWellKnown(windows.WinBuiltinAdministratorsSid) {
-		l.WarnContext(ctx, "working directory owner is not in administrators group")
+		l.WarnContext(ctx, "owner is not in administrators group")
 	}
 
-	err = rangeACEs(dacl, func(hdr windows.ACE_HEADER, mask windows.ACCESS_MASK, sid *windows.SID) (cont bool) {
-		l.DebugContext(ctx, "checking entry", "sid", sid, "mask", mask)
-
+	err = rangeACEs(dacl, func(
+		hdr windows.ACE_HEADER,
+		mask windows.ACCESS_MASK,
+		sid *windows.SID,
+	) (cont bool) {
 		warn := false
 		switch {
 		case hdr.AceType != windows.ACCESS_ALLOWED_ACE_TYPE:

internal/permcheck/migrate_unix.go

@@ -45,63 +45,63 @@ func migrate(
 	querylogDir string,
 	confFilePath string,
 ) {
-	chmodDir(ctx, l, workDir)
+	dirLoggger, fileLogger := l.With("type", typeDir), l.With("type", typeFile)
 
-	chmodFile(ctx, l, confFilePath)
+	chmodDir(ctx, dirLoggger, workDir)
+
+	chmodFile(ctx, fileLogger, confFilePath)
 
 	// TODO(a.garipov): Put all paths in one place and remove this duplication.
-	chmodDir(ctx, l, dataDir)
-	chmodDir(ctx, l, filepath.Join(dataDir, "filters"))
-	chmodFile(ctx, l, filepath.Join(dataDir, "sessions.db"))
-	chmodFile(ctx, l, filepath.Join(dataDir, "leases.json"))
+	chmodDir(ctx, dirLoggger, dataDir)
+	chmodDir(ctx, dirLoggger, filepath.Join(dataDir, "filters"))
+	chmodFile(ctx, fileLogger, filepath.Join(dataDir, "sessions.db"))
+	chmodFile(ctx, fileLogger, filepath.Join(dataDir, "leases.json"))
 
 	if dataDir != querylogDir {
-		chmodDir(ctx, l, querylogDir)
+		chmodDir(ctx, dirLoggger, querylogDir)
 	}
-	chmodFile(ctx, l, filepath.Join(querylogDir, "querylog.json"))
-	chmodFile(ctx, l, filepath.Join(querylogDir, "querylog.json.1"))
+	chmodFile(ctx, fileLogger, filepath.Join(querylogDir, "querylog.json"))
+	chmodFile(ctx, fileLogger, filepath.Join(querylogDir, "querylog.json.1"))
 
 	if dataDir != statsDir {
-		chmodDir(ctx, l, statsDir)
+		chmodDir(ctx, dirLoggger, statsDir)
 	}
-	chmodFile(ctx, l, filepath.Join(statsDir, "stats.db"))
+	chmodFile(ctx, fileLogger, filepath.Join(statsDir, "stats.db"))
 }
 
 // chmodDir changes the permissions of a single directory.  The results are
 // logged at the appropriate level.
 func chmodDir(ctx context.Context, l *slog.Logger, dirPath string) {
-	chmodPath(ctx, l, dirPath, typeDir, aghos.DefaultPermDir)
+	chmodPath(ctx, l, dirPath, aghos.DefaultPermDir)
 }
 
 // chmodFile changes the permissions of a single file.  The results are logged
 // at the appropriate level.
 func chmodFile(ctx context.Context, l *slog.Logger, filePath string) {
-	chmodPath(ctx, l, filePath, typeFile, aghos.DefaultPermFile)
+	chmodPath(ctx, l, filePath, aghos.DefaultPermFile)
 }
 
 // chmodPath changes the permissions of a single filesystem entity.  The results
 // are logged at the appropriate level.
-func chmodPath(ctx context.Context, l *slog.Logger, entPath, fileType string, fm fs.FileMode) {
-	switch err := os.Chmod(entPath, fm); {
+func chmodPath(ctx context.Context, l *slog.Logger, fpath string, fm fs.FileMode) {
+	var lvl slog.Level
+	var msg string
+	args := []any{"path", fpath}
+
+	switch err := os.Chmod(fpath, fm); {
 	case err == nil:
-		l.InfoContext(ctx, "changed permissions", "type", fileType, "path", entPath)
+		lvl = slog.LevelInfo
+		msg = "changed permissions"
 	case errors.Is(err, os.ErrNotExist):
-		l.DebugContext(
-			ctx,
-			"changing permissions",
-			"type", fileType,
-			"path", entPath,
-			slogutil.KeyError, err,
-		)
+		lvl = slog.LevelDebug
+		msg = "checking permissions"
+		args = append(args, slogutil.KeyError, err)
 	default:
-		l.ErrorContext(
-			ctx,
-			"can not change permissions; this can leave your system vulnerable, see "+
-				"https://adguard-dns.io/kb/adguard-home/running-securely/#os-service-concerns",
-			"type", fileType,
-			"path", entPath,
-			"target_perm", fmt.Sprintf("%#o", fm),
-			slogutil.KeyError, err,
-		)
+		lvl = slog.LevelError
+		msg = "can not change permissions; this can leave your system vulnerable, see " +
+			"https://adguard-dns.io/kb/adguard-home/running-securely/#os-service-concerns"
+		args = append(args, "target_perm", fmt.Sprintf("%#o", fm), slogutil.KeyError, err)
 	}
+
+	l.Log(ctx, lvl, msg, args...)
 }

internal/permcheck/migrate_windows.go

@@ -12,6 +12,8 @@ import (
 
 // needsMigration is the Windows-specific implementation of [NeedsMigration].
 func needsMigration(ctx context.Context, l *slog.Logger, workDir, _ string) (ok bool) {
+	l = l.With("type", typeDir, "path", workDir)
+
 	dacl, owner, err := getSecurityInfo(workDir)
 	if err != nil {
 		l.ErrorContext(ctx, "getting security info", slogutil.KeyError, err)
@@ -54,9 +56,11 @@ func needsMigration(ctx context.Context, l *slog.Logger, workDir, _ string) (ok
 
 // migrate is the Windows-specific implementation of [Migrate].
 func migrate(ctx context.Context, l *slog.Logger, workDir, _, _, _, _ string) {
+	dirLogger := l.With("type", typeDir, "path", workDir)
+
 	dacl, owner, err := getSecurityInfo(workDir)
 	if err != nil {
-		l.ErrorContext(ctx, "getting security info", slogutil.KeyError, err)
+		dirLogger.ErrorContext(ctx, "getting security info", slogutil.KeyError, err)
 
 		return
 	}
@@ -65,9 +69,10 @@ func migrate(ctx context.Context, l *slog.Logger, workDir, _, _, _, _ string) {
 		var admins *windows.SID
 		admins, err = windows.CreateWellKnownSid(windows.WinBuiltinAdministratorsSid)
 		if err != nil {
+			// This log message is not related to the directory.
 			l.ErrorContext(ctx, "creating administrators sid", slogutil.KeyError, err)
 		} else {
-			l.InfoContext(ctx, "migrating working directory owner", "sid", admins)
+			dirLogger.InfoContext(ctx, "migrating owner", "sid", admins)
 			owner = admins
 		}
 	}
@@ -82,26 +87,26 @@ func migrate(ctx context.Context, l *slog.Logger, workDir, _, _, _, _ string) {
 		switch {
 		case hdr.AceType != windows.ACCESS_ALLOWED_ACE_TYPE:
 			// Add non-allowed access control entries as is.
-			l.InfoContext(ctx, "migrating deny control entry", "sid", sid)
+			dirLogger.InfoContext(ctx, "migrating deny control entry", "sid", sid)
 			accessEntries = append(accessEntries, newDenyExplicitAccess(sid, mask))
 		case !sid.IsWellKnown(windows.WinBuiltinAdministratorsSid):
 			// Skip non-administrator ACEs.
-			l.InfoContext(ctx, "removing access control entry", "sid", sid)
+			dirLogger.InfoContext(ctx, "removing access control entry", "sid", sid)
 		default:
-			l.InfoContext(ctx, "migrating access control entry", "sid", sid, "mask", mask)
+			dirLogger.InfoContext(ctx, "migrating access control entry", "sid", sid, "mask", mask)
 			accessEntries = append(accessEntries, newFullExplicitAccess(sid))
 		}
 
 		return true
 	})
 	if err != nil {
-		l.ErrorContext(ctx, "ranging trough access control entries", slogutil.KeyError, err)
+		dirLogger.ErrorContext(ctx, "filtering access control entries", slogutil.KeyError, err)
 
 		return
 	}
 
 	err = setSecurityInfo(workDir, owner, accessEntries)
 	if err != nil {
-		l.ErrorContext(ctx, "setting security info", slogutil.KeyError, err)
+		dirLogger.ErrorContext(ctx, "setting security info", slogutil.KeyError, err)
 	}
 }

internal/permcheck/permcheck.go

@@ -8,8 +8,6 @@ import (
 )
 
 // File type constants for logging.
-//
-// TODO(e.burkov):  !! Use in Windows logging.
 const (
 	typeDir  = "directory"
 	typeFile = "file"

internal/permcheck/security_windows.go

@@ -16,6 +16,8 @@ const securityInfo windows.SECURITY_INFORMATION = windows.OWNER_SECURITY_INFORMA
 	windows.PROTECTED_DACL_SECURITY_INFORMATION |
 	windows.UNPROTECTED_DACL_SECURITY_INFORMATION
 
+// objectType is the type of the object for directories in context of security
+// API.
 const objectType = windows.SE_FILE_OBJECT
 
 // fileDeleteChildRight is the mask bit for the right to delete a child object.
@@ -41,8 +43,8 @@ const fullControlMask windows.ACCESS_MASK = windows.FILE_LIST_DIRECTORY |
 	windows.SYNCHRONIZE
 
 // aceFunc is a function that handles access control entries in the
-// discretionary access control list.  It should return true to continue ranging
-// or false to stop.
+// discretionary access control list.  It should return true to continue
+// iterating over the entries, or false to stop.
 type aceFunc = func(
 	hdr windows.ACE_HEADER,
 	mask windows.ACCESS_MASK,
@@ -50,7 +52,7 @@ type aceFunc = func(
 ) (cont bool)
 
 // rangeACEs ranges over the access control entries in the discretionary access
-// control list of the specified security descriptor.
+// control list of the specified security descriptor and calls f for each one.
 func rangeACEs(dacl *windows.ACL, f aceFunc) (err error) {
 	var errs []error
 	for i := range uint32(dacl.AceCount) {
@@ -119,15 +121,14 @@ func getSecurityInfo(fname string) (dacl *windows.ACL, owner *windows.SID, err e
 
 // newFullExplicitAccess creates a new explicit access entry with full control
 // permissions.
-func newFullExplicitAccess(sid *windows.SID) (expAcc windows.EXPLICIT_ACCESS) {
-	// TODO(e.burkov):  !! lookup account type
+func newFullExplicitAccess(sid *windows.SID) (accEnt windows.EXPLICIT_ACCESS) {
 	return windows.EXPLICIT_ACCESS{
 		AccessPermissions: fullControlMask,
 		AccessMode:        windows.GRANT_ACCESS,
 		Inheritance:       windows.SUB_CONTAINERS_AND_OBJECTS_INHERIT,
 		Trustee: windows.TRUSTEE{
 			TrusteeForm:  windows.TRUSTEE_IS_SID,
-			TrusteeType:  windows.TRUSTEE_IS_GROUP,
+			TrusteeType:  windows.TRUSTEE_IS_UNKNOWN,
 			TrusteeValue: windows.TrusteeValueFromSID(sid),
 		},
 	}
@@ -135,15 +136,17 @@ func newFullExplicitAccess(sid *windows.SID) (expAcc windows.EXPLICIT_ACCESS) {
 
 // newDenyExplicitAccess creates a new explicit access entry with specified deny
 // permissions.
-func newDenyExplicitAccess(sid *windows.SID, mask windows.ACCESS_MASK) (expAcc windows.EXPLICIT_ACCESS) {
-	// TODO(e.burkov):  !! lookup account type
+func newDenyExplicitAccess(
+	sid *windows.SID,
+	mask windows.ACCESS_MASK,
+) (accEnt windows.EXPLICIT_ACCESS) {
 	return windows.EXPLICIT_ACCESS{
 		AccessPermissions: mask,
 		AccessMode:        windows.DENY_ACCESS,
 		Inheritance:       windows.SUB_CONTAINERS_AND_OBJECTS_INHERIT,
 		Trustee: windows.TRUSTEE{
 			TrusteeForm:  windows.TRUSTEE_IS_SID,
-			TrusteeType:  windows.TRUSTEE_IS_GROUP,
+			TrusteeType:  windows.TRUSTEE_IS_UNKNOWN,
 			TrusteeValue: windows.TrusteeValueFromSID(sid),
 		},
 	}