Proposal: Handling default settings for SSHD configuration resource

[michaeltlombardi]

Summary of the new feature / enhancement

Context

Following is a comment on the proposed implementation for the get operation of the sshdconfig resource (see #1004):

• if no input is provided, get returns all settings from SSHD -T including defaults
• if input is provided, get returns the subset of settings from SSHD -T included in the input
• if defaults: false is passed in via _metadata, get returns the subset of settings from SSHD -T that are not defaults
• if filepath: <filepath> is passed in via _metadata, get runs SSHD -T -f <filepath>

The differing behavior for input/no-input in the get operation here is a break from prior design, user expectation, and integration with higher order tooling. Typically, get operations return the full configurable state for a resource instance. This filtering makes more sense to me for the export operation. This design also prevents me from retrieving the full configuration but only verifying or enforcing a specific subset.

Similarly, here we seem to be defining write-only properties (behavior-modifying parameters) to be passed through _metadata. Arguably, filepath is a non-required key property - I would expect a configuration document containing any two instances of this resource with the same filepath (or filepath not being defined) to raise a conflict in the future.

Originally posted by @michaeltlombardi in #1004 (comment)

Proposal

Circling back to this from a design discussion, I would propose that the resource:

• Define the read-only defaultSettings property as an array of property names.
• Indicate whether the resource should explicitly export default settings with a write-only exportDefaults property.

Note

I'm using the property names defaultSettings and exportDefaults for convenience. The actual implemented names should be more thoroughly discussed.

Output for the get operation

When the get operation is invoked¹, the resource returns every configuration setting. The defaultSettings property indicates which settings were provided as defaults by SSHD.

sshdconfig get

port:
- 22
addressfamily: any
listenaddress:
- 0.0.0.0:22
- '[::]:22'
logingracetime: 120
x11displayoffset: 10
maxauthtries: 6
syslogfacility: LOCAL0
authorizedkeysfile:
- .ssh/authorized_keys
defaultSettings:
- port
- addressfamily
- listenaddress
- logingracetime
- x11displayoffset
- maxauthtries

Behavior for export

When defining the resource for the export operation, the exportDefaults write-only property changes the result object:

• Without the exportDefaults property defined, the output includes only the explicitly set properties:

  sshdconfig export

  syslogfacility: LOCAL0
  authorizedkeysfile:
  - .ssh/authorized_keys

• With exportDefaults defined as true, the export operation returns the effective configuration - when used in set, it will explicitly define each of these settings:

  sshdconfig export --input '{"exportDefaults": true}'

  port:
  - 22
  addressfamily: any
  listenaddress:
  - 0.0.0.0:22
  - '[::]:22'
  logingracetime: 120
  x11displayoffset: 10
  maxauthtries: 6
  syslogfacility: LOCAL0
  authorizedkeysfile:
  - .ssh/authorized_keys

  Note that it doesn't define the defaultSettings property - because for the exported instance, none of the settings are default.

  The documentation for the exportDefaults write-only property should indicate that it's ignored for all non-export operations.².

Alternative proposal - _metadata

Alternatively, the defaultSettings read-only property could be returned as metadata. I prefer not to follow this model, as it seems effectively the same, but with an additional level of nesting:

port:
- 22
addressfamily: any
listenaddress:
- 0.0.0.0:22
- '[::]:22'
logingracetime: 120
x11displayoffset: 10
maxauthtries: 6
syslogfacility: LOCAL0
authorizedkeysfile:
- .ssh/authorized_keys
_metadata:
  defaultSettings:
  - port
  - addressfamily
  - listenaddress
  - logingracetime
  - x11displayoffset
  - maxauthtries

I would prefer that we reserve using _metadata for things that can't effectively be modeled by the resource, or provide context external to it (such as the current security context). In this case, which settings are provided as defaults by SSHD is knowable and describable from the resource surface.

I think we would still need to define the write-only exportDefaults property. Similarly, we could nest it under the _metadata field, but I'm not sure whether this improves the user experience.

Proposed technical implementation details (optional)

No response

Footnotes

1. I'm only showing the example output for the get operation here, but I would expect that the defaultSettings read-only property would also be populated in the actual state for test and the before/after state for set - this would also help distinguish changes where a default setting was explicitly defined to the same or a different value. ↩

2. In the future, we may want to introduce a keyword to our schema vocabulary that limits which operations a property is valid for, but I don't think it's currently necessary. Having that keyword could improve documentation/validation, but for now I think annotating the usage in documentation is sufficient. ↩

[SteveL-MSFT]

I think any "properties" that affect resource behavior, but not part of what the resource is managing should be in metadata, as such includeDefaults, for example, would be metadata. For the actual data, I wonder if inheritedDefaultSettings might be a better descriptive name of the read-only container object. Unfortunately, JSON doesn't have attributes, otherwise I'd go with attributing properties to indicate they were inherited defaults. As such, I tend to think of this information as metadata and would prefer it be in the metadata section. Would welcome other opinions.

[michaeltlombardi]

I think any "properties" that affect resource behavior, but not part of what the resource is managing should be in metadata

This is contrary to prior documented design for write-only properties (emphasis added):

Resources can define write-only properties that affect how the resource behaves but that the resource can't retrieve for the actual state of an instance. For example, a resource might support credentials for downloading a file, but won't return those credentials in the output.

In this case, exportDefaults or includeDefaults fall under the documented design for write-only properties and are representable in the default JSON Schema vocabulary. I think if we implemented them in the _metadata we would still need to mark the field as write-only and also clearly document behavior.

Regarding the inheritedDefaultSettings:

• Agree that this name is more semantically helpful.
• It could arguably be returned as metadata, but I don't see a meaningful difference between providing this read-only metadata and the last write time for a file resource instance. In neither case can you set the value of the returned data, but it's information about the instance itself.

[tgauth]

The name inheritedDefaultSettings brings up the edge case where a setting is explicitly defined in sshd_config and also happens to be the implicit default in sshd (i.e. setting Port 22).

Should there be a differentiation between implicit and explicit defaults or is it sufficient to group them as defaults?
I think that differentiation might not be a high-priority requirement for the user at this point (perhaps an enhancement, in the future). If they are grouped together, just defaultSettings may be more clear.

[michaeltlombardi]

Should there be a differentiation between implicit and explicit defaults or is it sufficient to group them as defaults?

@tgauth - in this case, I would expect the explicitly defined port to not appear in inheritedDefaultSettings - It happens to be explicitly defined to the same value as the default, but it's not inherited.

I think the documentation/design should indicate to a user that you can explicitly define settings to their default value as a safeguard against changing defaults in the future, but that you would need to unset them to revert to the default value. This could be modeled in a few ways with various tradeoffs (non-exhaustive list):

1. If a property is defined in the desired state as null, we want to unset the property. This can only really work if null isn't a valid value for properties.
2. We define a special enum value, like _inheritDefault to indicate that the property shouldn't be explcitly defined. This can get a little messy for test operations and reading output state.
3. We turn inheritedDefaultSettings into a read-write property, where if you define it in the desired state you're indicating that you want that property to be inherited, not explicitly set, and forbid also defining a desired state for properties in that array.

[tgauth]

1. If a property is defined in the desired state as null, we want to unset the property. This can only really work if null isn't a valid value for properties.

null won't be a valid value for a property since it's all being translated back to text, so this could work, but none as a string value is valid for multiple properties, so that might cause confusion.

3. We turn inheritedDefaultSettings into a read-write property, where if you define it in the desired state you're indicating that you want that property to be inherited, not explicitly set, and forbid also defining a desired state for properties in that array.

I like that this clearly delineates what's being inherited vs. explicitly set, though it may be cumbersome to move a property from one to the other.

[michaeltlombardi]

I like that this clearly delineates what's being inherited vs. explicitly set, though it may be cumbersome to move a property from one to the other.

I think the primary issue is around the (probable) length of the inheritedDefaultSettings property. Even in this subset snippet, there's six inherited settings:

port:
- 22
addressfamily: any
listenaddress:
- 0.0.0.0:22
- '[::]:22'
logingracetime: 120
x11displayoffset: 10
maxauthtries: 6
syslogfacility: LOCAL0
authorizedkeysfile:
- .ssh/authorized_keys
inheritedDefaultSettings:
- port
- addressfamily
- listenaddress
- logingracetime
- x11displayoffset
- maxauthtries

Deciding to make port explicit would require the following desired state:

port:
- 22
addressfamily: any
listenaddress:
- 0.0.0.0:22
- '[::]:22'
logingracetime: 120
x11displayoffset: 10
maxauthtries: 6
syslogfacility: LOCAL0
authorizedkeysfile:
- .ssh/authorized_keys
inheritedDefaultSettings:
- addressfamily
- listenaddress
- logingracetime
- x11displayoffset
- maxauthtries

Which is even less enjoyable to author if there's dozens of inherited settings. I don't think adding a separate property like overrideDefaultSettings would be very ergonomic, especially considering the default assumption is probably that "anything I specified is explicitly set." We would probably need to clarify that specifying inheritedDefaultSettings in the desired state is optional and clearly indicate its behavior (anything included in that array will only be retrieved/compared, not set).

[SteveL-MSFT]

Perhaps this is where we reconsider having inheritedDefaultSettings as metadata since it's more obvious that it's not something you set (even though some metadata is settable)?

[tgauth]

notes from discussion:

1. _inheritedDefaultSettings and _includeDefaults will be canonical properties
2. For get, the default behavior for _includeDefaults will be true
3. For export, the default behavior for _includeDefaults will be false

[SteveL-MSFT]

notes from discussion:

1. _inheritedDefaultSettings and _includeDefaults will be canonical properties
2. For get, the default behavior for _includeDefaults will be true
3. For export, the default behavior for _includeDefaults will be false

I think as discussed it's just _inheritedDefaults (sans Settings as it's implied)