elastic / kibana

Your window into the Elastic Stack
https://www.elastic.co/products/kibana
Other
19.49k stars 8.05k forks source link

[Cloud Security] Refactor `onSetupFormatChange` #164978

Open JordanSh opened 10 months ago

JordanSh commented 10 months ago

The current credential type switch logic is using onSetupFormatChange on change. it has been identified as having certain issues when invoked with the same credential type, can lead to problems like data deletion or locking of the previous type.

To address this technical debt, a refactoring of the onSetupFormatChange is required. The goal is to enhance the function's behavior to handle cases of the same credential type more gracefully and prevent unintended side effects.

elasticmachine commented 10 months ago

Pinging @elastic/kibana-cloud-security-posture (Team:Cloud Security)

kfirpeled commented 10 months ago

@JordanSh what's the estimation for it?

JordanSh commented 10 months ago

hard to tell, id say 5-8. wdyt @opauloh ?

opauloh commented 10 months ago

I say it's 5 if we are going to apply the fix here on the onSetupChange itself (compare the data is changed before calling the updatePolicy)

the function has this structure:

const onSetupFormatChange = (newSetupFormat: SetupFormat) => {
    if (newSetupFormat === 'cloud_formation') {
      // We need to store the current manual fields to restore them later
      fieldsSnapshot.current = Object.fromEntries(
        fields.map((field) => [field.id, { value: field.value }])
      );
      // We need to store the last manual credentials type to restore it later
      lastManualCredentialsType.current = getAwsCredentialsType(input);

      updatePolicy(
        getPosturePolicy(newPolicy, input.type, {
          'aws.credentials.type': {
            value: 'cloud_formation',
            type: 'text',
          },
          // Clearing fields from previous setup format to prevent exposing credentials
          // when switching from manual to cloud formation
          ...Object.fromEntries(fields.map((field) => [field.id, { value: undefined }])),
        })
      );
    } else {
      updatePolicy(
        getPosturePolicy(newPolicy, input.type, {
          'aws.credentials.type': {
            // Restoring last manual credentials type or defaulting to the first option
            value: lastManualCredentialsType.current || DEFAULT_MANUAL_AWS_CREDENTIALS_TYPE,
            type: 'text',
          },
          // Restoring fields from manual setup format if any
          ...fieldsSnapshot.current,
        })
      );
    }
  };

It seems that lacking the comparison can lead to an out-of-sync state between the newPolicy state and the useRef object.

Also worth applying the same fix for the GCP onSetupFormatChange

opauloh commented 10 months ago

Alternatively, if I understood correctly the manual fields are being cleared on the backend now (as once mentioned by @mitodrummer on the last Kibana guild), in that case, we can remove the need for the onSetupFormatChange function itself, and also apply the same backend method for GCP which is my preferred way of solving this.