Merge pull request #533 from safing/fix/profile-updates-and-deletion

Fix profile updates and deletion

firewall/prompt.go

@@ -259,7 +259,7 @@ func saveResponse(p *profile.Profile, entity *intel.Entity, promptResponse strin
 	// Update the profile if necessary.
 	if p.IsOutdated() {
 		var err error
-		p, err = profile.GetProfile(p.Source, p.ID, p.LinkedPath)
+		p, err = profile.GetProfile(p.Source, p.ID, p.LinkedPath, false)
 		if err != nil {
 			return err
 		}

process/find.go

@@ -64,7 +64,7 @@ func GetNetworkHost(ctx context.Context, remoteIP net.IP) (process *Process, err
 	}
 
 	// Get the (linked) local profile.
-	networkHostProfile, err := profile.GetProfile(profile.SourceNetwork, remoteIP.String(), "")
+	networkHostProfile, err := profile.GetProfile(profile.SourceNetwork, remoteIP.String(), "", false)
 	if err != nil {
 		return nil, err
 	}

process/profile.go

@@ -81,7 +81,7 @@ func (p *Process) GetProfile(ctx context.Context) (changed bool, err error) {
 	}
 
 	// Get the (linked) local profile.
-	localProfile, err := profile.GetProfile(profile.SourceLocal, profileID, p.Path)
+	localProfile, err := profile.GetProfile(profile.SourceLocal, profileID, p.Path, false)
 	if err != nil {
 		return false, err
 	}
@@ -105,11 +105,8 @@ func (p *Process) UpdateProfileMetadata() {
 	// Update metadata of profile.
 	metadataUpdated := localProfile.UpdateMetadata(p.Path)
 
-	// Mark profile as used.
-	profileChanged := localProfile.MarkUsed()
-
 	// Save the profile if we changed something.
-	if metadataUpdated || profileChanged {
+	if metadataUpdated {
 		err := localProfile.Save()
 		if err != nil {
 			log.Warningf("process: failed to save profile %s: %s", localProfile.ScopedID(), err)

profile/database.go

@@ -61,27 +61,36 @@ func startProfileUpdateChecker() error {
 				// Get active profile.
 				activeProfile := getActiveProfile(strings.TrimPrefix(r.Key(), profilesDBPath))
 				if activeProfile == nil {
+					// Don't do any additional actions if the profile is not active.
 					continue profileFeed
 				}
 
-				// If the record is being deleted, but there is an active profile,
+				// If the record is being deleted, reset the profile.
 				// create an empty profile instead.
 				if r.Meta().IsDeleted() {
-					newProfile := New(
+					newProfile, err := GetProfile(
 						activeProfile.Source,
 						activeProfile.ID,
 						activeProfile.LinkedPath,
-						nil,
+						true,
 					)
-					// Copy some metadata from the old profile.
-					newProfile.Name = activeProfile.Name
-					// Save the new profile.
-					err := newProfile.Save()
 					if err != nil {
-						log.Errorf("profile: failed to save new profile for profile reset: %s", err)
+						log.Errorf("profile: failed to create new profile after reset: %s", err)
+					} else {
+						// Copy metadata from the old profile.
+						newProfile.copyMetadataFrom(activeProfile)
+						// Save the new profile.
+						err = newProfile.Save()
+						if err != nil {
+							log.Errorf("profile: failed to save new profile after reset: %s", err)
+						}
 					}
-					// Set to outdated, so it is loaded in the layered profiles.
+
+					// If the new profile was successfully created, update layered profile.
 					activeProfile.outdated.Set()
+					if err == nil {
+						newProfile.layeredProfile.Update()
+					}
 				}
 
 				// Always increase the revision counter of the layer profile.
@@ -125,12 +134,8 @@ func (h *databaseHook) PrePut(r record.Record) (record.Record, error) {
 	// clean config
 	config.CleanHierarchicalConfig(profile.Config)
 
-	// prepare config
-	err = profile.prepConfig()
-	if err != nil {
-		// error here, warning when loading
-		return nil, err
-	}
+	// prepare profile
+	profile.prepProfile()
 
 	// parse config
 	err = profile.parseConfig()

profile/get.go

@@ -14,10 +14,8 @@ var getProfileLock sync.Mutex
 
 // GetProfile fetches a profile. This function ensures that the loaded profile
 // is shared among all callers. You must always supply both the scopedID and
-// linkedPath parameters whenever available. The linkedPath is used as the key
-// for locking concurrent requests, so it must be supplied if available.
-// If linkedPath is not supplied, source and id make up the key instead.
-func GetProfile(source profileSource, id, linkedPath string) ( //nolint:gocognit
+// linkedPath parameters whenever available.
+func GetProfile(source profileSource, id, linkedPath string, reset bool) ( //nolint:gocognit
 	profile *Profile,
 	err error,
 ) {
@@ -40,28 +38,23 @@ func GetProfile(source profileSource, id, linkedPath string) ( //nolint:gocognit
 		if profile != nil {
 			profile.MarkStillActive()
 
-			if profile.outdated.IsSet() {
+			if profile.outdated.IsSet() || reset {
 				previousVersion = profile
 			} else {
 				return profile, nil
 			}
 		}
+
 		// Get from database.
-		profile, err = getProfile(scopedID)
-
-		// Check if the request is for a special profile that may need a reset.
-		if err == nil && specialProfileNeedsReset(profile) {
-			// Trigger creation of special profile.
-			err = database.ErrNotFound
-		}
-
-		// If we cannot find a profile, check if the request is for a special
-		// profile we can create.
-		if errors.Is(err, database.ErrNotFound) {
-			profile = getSpecialProfile(id, linkedPath)
-			if profile != nil {
-				err = nil
+		if !reset {
+			profile, err = getProfile(scopedID)
+			// Check if the profile is special and needs a reset.
+			if err == nil && specialProfileNeedsReset(profile) {
+				profile = getSpecialProfile(id, linkedPath)
 			}
+		} else {
+			// Simulate missing profile to create new one.
+			err = database.ErrNotFound
 		}
 
 	case linkedPath != "":
@@ -70,28 +63,54 @@ func GetProfile(source profileSource, id, linkedPath string) ( //nolint:gocognit
 		// the linked path.
 		profile = findActiveProfile(linkedPath)
 		if profile != nil {
-			if profile.outdated.IsSet() {
+			if profile.outdated.IsSet() || reset {
 				previousVersion = profile
 			} else {
 				return profile, nil
 			}
 		}
+
 		// Get from database.
-		profile, err = findProfile(linkedPath)
+		if !reset {
+			profile, err = findProfile(linkedPath)
+			// Check if the profile is special and needs a reset.
+			if err == nil && specialProfileNeedsReset(profile) {
+				profile = getSpecialProfile(id, linkedPath)
+			}
+		} else {
+			// Simulate missing profile to create new one.
+			err = database.ErrNotFound
+		}
 
 	default:
 		return nil, errors.New("cannot fetch profile without ID or path")
 	}
+
+	// Create new profile if none was found.
+	if errors.Is(err, database.ErrNotFound) {
+		err = nil
+
+		// Check if there is a special profile for this ID.
+		profile = getSpecialProfile(id, linkedPath)
+
+		// If not, create a standard profile.
+		if profile == nil {
+			profile = New(SourceLocal, id, linkedPath, nil)
+		}
+	}
+
+	// If there was a non-recoverable error, return here.
 	if err != nil {
 		return nil, err
 	}
 
-	// Process profiles coming directly from the database.
+	// Process profiles are coming directly from the database or are new.
 	// As we don't use any caching, these will be new objects.
 
 	// Add a layeredProfile to local and network profiles.
 	if profile.Source == SourceLocal || profile.Source == SourceNetwork {
 		// If we are refetching, assign the layered profile from the previous version.
+		// The internal references will be updated when the layered profile checks for updates.
 		if previousVersion != nil {
 			profile.layeredProfile = previousVersion.layeredProfile
 		}
@@ -158,11 +177,8 @@ func prepProfile(r record.Record) (*Profile, error) {
 		return nil, err
 	}
 
-	// prepare config
-	err = profile.prepConfig()
-	if err != nil {
-		log.Errorf("profiles: profile %s has (partly) invalid configuration: %s", profile.ID, err)
-	}
+	// prepare profile
+	profile.prepProfile()
 
 	// parse config
 	err = profile.parseConfig()

profile/profile-layered.go

@@ -238,13 +238,19 @@ func (lp *LayeredProfile) Update() (revisionCounter uint64) {
 	for i, layer := range lp.layers {
 		if layer.outdated.IsSet() {
 			changed = true
-			// update layer
-			newLayer, err := GetProfile(layer.Source, layer.ID, layer.LinkedPath)
+
+			// Update layer.
+			newLayer, err := GetProfile(layer.Source, layer.ID, layer.LinkedPath, false)
 			if err != nil {
-				log.Errorf("profiles: failed to update profile %s", layer.ScopedID())
+				log.Errorf("profiles: failed to update profile %s: %s", layer.ScopedID(), err)
 			} else {
 				lp.layers[i] = newLayer
 			}
+
+			// Update local profile reference.
+			if i == 0 {
+				lp.localProfile = newLayer
+			}
 		}
 	}
 	if !lp.globalValidityFlag.IsValid() {
@@ -276,11 +282,6 @@ func (lp *LayeredProfile) updateCaches() {
 	atomic.StoreUint32(lp.securityLevel, uint32(newLevel))
 }
 
-// MarkUsed marks the localProfile as used.
-func (lp *LayeredProfile) MarkUsed() {
-	lp.localProfile.MarkUsed()
-}
-
 // SecurityLevel returns the highest security level of all layered profiles. This function is atomic and does not require any locking.
 func (lp *LayeredProfile) SecurityLevel() uint8 {
 	return uint8(atomic.LoadUint32(lp.securityLevel))

profile/profile.go

@@ -137,28 +137,27 @@ type Profile struct { //nolint:maligned // not worth the effort
 	savedInternally bool
 }
 
-func (profile *Profile) prepConfig() (err error) {
+func (profile *Profile) prepProfile() {
 	// prepare configuration
-	profile.configPerspective, err = config.NewPerspective(profile.Config)
 	profile.outdated = abool.New()
 	profile.lastActive = new(int64)
-	return
 }
 
 func (profile *Profile) parseConfig() error {
-	if profile.configPerspective == nil {
-		return errors.New("config not prepared")
-	}
-
-	// check if already parsed
+	// Check if already parsed.
 	if profile.dataParsed {
 		return nil
 	}
+
+	// Create new perspective and marked as parsed.
+	var err error
+	profile.configPerspective, err = config.NewPerspective(profile.Config)
+	if err != nil {
+		return fmt.Errorf("failed to create config perspective: %w", err)
+	}
 	profile.dataParsed = true
 
-	var err error
 	var lastErr error
-
 	action, ok := profile.configPerspective.GetAsString(CfgOptionDefaultActionKey)
 	profile.defaultAction = DefaultActionNotSet
 	if ok {
@@ -238,9 +237,7 @@ func New(
 	profile.makeKey()
 
 	// Prepare and parse initial profile config.
-	if err := profile.prepConfig(); err != nil {
-		log.Errorf("profile: failed to prep new profile: %s", err)
-	}
+	profile.prepProfile()
 	if err := profile.parseConfig(); err != nil {
 		log.Errorf("profile: failed to parse new profile: %s", err)
 	}
@@ -281,30 +278,6 @@ func (profile *Profile) LastActive() int64 {
 	return atomic.LoadInt64(profile.lastActive)
 }
 
-// MarkUsed updates ApproxLastUsed when it's been a while and saves the profile if it was changed.
-func (profile *Profile) MarkUsed() (changed bool) {
-	/*
-		TODO:
-		This might be one of the things causing problems with disappearing settings.
-		Possibly this is called with an outdated profile and then kills settings
-		already in the database.
-		Generally, it probably causes more harm than good if we periodically touch
-		the most important database entries just to update a timestamp.
-		We should save this data elsewhere and make configuration data as stable as
-		possible.
-
-		profile.Lock()
-		defer profile.Unlock()
-
-		if time.Now().Add(-lastUsedUpdateThreshold).Unix() > profile.ApproxLastUsed {
-			profile.ApproxLastUsed = time.Now().Unix()
-			return true
-		}
-	*/
-
-	return false
-}
-
 // String returns a string representation of the Profile.
 func (profile *Profile) String() string {
 	return fmt.Sprintf("<%s %s/%s>", profile.Name, profile.Source, profile.ID)
@@ -469,6 +442,31 @@ func (profile *Profile) UpdateMetadata(binaryPath string) (changed bool) {
 	return changed
 }
 
+func (profile *Profile) copyMetadataFrom(otherProfile *Profile) (changed bool) {
+	if profile.Name != otherProfile.Name {
+		profile.Name = otherProfile.Name
+		changed = true
+	}
+	if profile.Description != otherProfile.Description {
+		profile.Description = otherProfile.Description
+		changed = true
+	}
+	if profile.Homepage != otherProfile.Homepage {
+		profile.Homepage = otherProfile.Homepage
+		changed = true
+	}
+	if profile.Icon != otherProfile.Icon {
+		profile.Icon = otherProfile.Icon
+		changed = true
+	}
+	if profile.IconType != otherProfile.IconType {
+		profile.IconType = otherProfile.IconType
+		changed = true
+	}
+
+	return
+}
+
 // updateMetadataFromSystem updates the profile metadata with data from the
 // operating system and saves it afterwards.
 func (profile *Profile) updateMetadataFromSystem(ctx context.Context) error {

profile/special.go

@@ -36,6 +36,8 @@ In order to respect the app settings of the actual application, DNS requests fro
 - Outgoing Rules (without global rules)
 - Block Bypassing
 - Filter Lists
+
+If you think you might have messed up the settings of the System DNS Client, just delete the profile below to reset it to the defaults.
 `
 
 	// PortmasterProfileID is the profile ID used for the Portmaster Core itself.
@@ -193,6 +195,10 @@ func getSpecialProfile(profileID, linkedPath string) *Profile {
 // check if the special profile has not been changed by the user and if not,
 // check if the profile is outdated and can be upgraded.
 func specialProfileNeedsReset(profile *Profile) bool {
+	if profile == nil {
+		return false
+	}
+
 	switch {
 	case profile.Source != SourceLocal:
 		// Special profiles live in the local scope only.

status/config.go

@@ -19,6 +19,7 @@ func registerConfig() error {
 		DefaultValue:   false,
 		Annotations: config.Annotations{
 			config.DisplayOrderAnnotation: 514,
+			config.CategoryAnnotation:     "User Interface",
 		},
 	}); err != nil {
 		return err