Codecov Report

Attention: Patch coverage is 77.31092% with 81 lines in your changes missing coverage. Please review.

Project coverage is 72.42%. Comparing base (441577b) to head (32232e6).
Report is 1 commits behind head on main.

@@            Coverage Diff             @@
##             main    #5357      +/-   ##
==========================================
+ Coverage   72.33%   72.42%   +0.08%     
==========================================
  Files         384      388       +4     
  Lines       23820    24177     +357     
  Branches     3669     3706      +37     
==========================================
+ Hits        17231    17509     +278     
- Misses       4785     4850      +65     
- Partials     1804     1818      +14     

@divyanshi-0402 Thanks for working on this.

Maybe this is a good opportunity to change the configuration process to instead add a new version in a single index (Great opportunity to rename our index!) and when loading the configuration the plugin loads specific versions from the set of all changes. For rollback this would mean just changing pointers rather than having to change the data itself.

Here is a diagram showing how we'd move between immutable versions of the configuration. I'll be curious what you think of this approach.

flowchart LR
  subgraph .security_config
    v1["v1"]
    v2["v2"]
    v3["v3"]
  end

  Load["Active Security Config"]

  %% Initial pointer to v3
  Load --> v3

  %% Rollback pointer to v2 (dashed red)
  Load -. rollback .-> v2

  %% Style the rollback link
  linkStyle 1 stroke:#f66,stroke-width:2px,stroke-dasharray:5,5

Thank you for the PR @divyanshi-0402 ! I will take a pass shortly and provide some comments.

On a high-level, I think we should store the diffs instead of the full snapshots as there would be a lot of redundant information stored for each snapshot of the security config for small changes. I believe that's how collaborative word editors work where they keep track of the diffs in order to implement undo and redo without having to keep snapshots of the document being edited. For clusters where the security index is small, snapshots may be performant but we should design this regardless of the size of the security index and support cases where the security index could contain MBs of data with more frequent changes.

The audit log does have capability to keep track of diffs to the security index using the COMPLIANCE_INTERNAL_CONFIG_WRITE auditlog category and computes the diffs here.

On a high-level, I think we should store the diffs instead of the full snapshots as there would be a lot of redundant information stored for each snapshot of the security config for small changes. I believe that's how collaborative word editors work where they keep track of the diffs in order to implement undo and redo without having to keep snapshots of the document being edited. For clusters where the security index is small, snapshots may be performant but we should design this regardless of the size of the security index and support cases where the security index could contain MBs of data with more frequent changes.

Storing the diffs/incremental changes can be helpful to reduce storage overhead. I think for now allowing users to configure versions to retain is also a good start. One problem with just storing diffs is that it would require more logic to restore versions or for users to look at different versions of the security config. For example, for a search request on the index that stores versions, this would mean using the current config and adding/removing configs (replay the changes) to show a meaningful response OR maybe showing the diff is okay for now. I think the current mindset is to support versioning of configs - however, we should allow customers to enable this and dynamically change number of versions to store.

@divyanshi-0402 Thanks for working on this.

Maybe this is a good opportunity to change the configuration process to instead add a new version in a single index (Great opportunity to rename our index!) and when loading the configuration the plugin loads specific versions from the set of all changes. For rollback this would mean just changing pointers rather than having to change the data itself.

@peternied Thanks for you suggestions!
While consolidating into a single index is an interesting idea, updating .opendistro_security directly would require widespread changes across the entire codebase and carries a higher risk of introducing instability. From an architectural perspective, keeping version history in a separate index helps isolate configuration versions, and reduces the risk of corrupting active security configurations.

One question: In the PR description, you describe your manual testing steps. Could you give also the specs of the cluster on which you did the manual testing? Especially, I would be interested in the number of nodes in the cluster.

couple of high-level questions- (let me know if you have already addressed these elsewhere)

1. How does this handle conflict between new write while trying to store previous version?
2. Is security configuration reloaded in-memory during rollback? Does that also trigger storing a new version, if yes, this could lead to a infinite loop? (new update -> save version -> triggers reload/updates security index ->?? treats this as a new update and starts saving it again) If no, how is it avoided?
3. This is more for the next PR, View API and Rollback API for Versioned Security Configurations #5366 - can the permission to view/rollback be delegated?
4. Is the new index protected at same level as security index? for e.g. users without access to security index/security roles, should not be able to execute a search on the version history index.

One question: In the PR description, you describe your manual testing steps. Could you give also the specs of the cluster on which you did the manual testing? Especially, I would be interested in the number of nodes in the cluster.

We have tested in a single node cluster, will be doing testing with multiple nodes as well.

couple of high-level questions- (let me know if you have already addressed these elsewhere)

1. How does this handle conflict between new write while trying to store previous version?
2. Is security configuration reloaded in-memory during rollback? Does that also trigger storing a new version, if yes, this could lead to a infinite loop? (new update -> save version -> triggers reload/updates security index ->?? treats this as a new update and starts saving it again) If no, how is it avoided?
3. This is more for the next PR, View API and Rollback API for Versioned Security Configurations #5366 - can the permission to view/rollback be delegated?
4. Is the new index protected at same level as security index? for e.g. users without access to security index/security roles, should not be able to execute a search on the version history index.

1. In case of version conflict we handle it through exception.
2. After the opendistro_security index is updated, we update the version index with the new version, there's no infinite loop.
3. Ack, will be adding the permissions
4. Yes the permission evaluation is done.

We have tested in a single node cluster, will be doing testing with multiple nodes as well.

To be honest, IMHO, a multi node test is absolutely essential, especially if there are no integration tests.

@divyanshi-0402 thank you for replying to and addressing the feedback in a timely manner.

I think any outstanding comments can be addressed in a follow-up PR.

+1, Thanks @divyanshi-0402! Can you or @nagarajg17 create a GH issue to keep track of pending comments/changes/follow-ups etc?

@shikharj05, @cwperks, @nibix Tracking the follow up items here #5407
cc - @divyanshi-0402

Test details for multi-node testing:

Created two nodes : opensearch-node1 and opensearch-node1

node 1 :

./bin/opensearch -E cluster.name=my-cluster -E node.name=node1 -E path.data=./data1 -E http.port=9200 -E transport.port=9300

[node1] Successfully saved version v1 to .opensearch_security_config_versions

[INFO ][o.o.s.c.ConfigurationRepository] [node1] Node 'node1' initialized

node 2 :

./bin/opensearch -E cluster.name=my-cluster -E node.name=node2 -E path.data=./data2 -E http.port=9201 -E transport.port=9301

[INFO ][o.o.s.c.ConfigurationRepository] [node2] Node 'node2' initialized

Updated Security Configurations from node 1 :

curl -XPUT https://localhost:9200/_plugins/_security/api/internalusers/testuser -u 'admin:admin' --insecure -H 'Content-Type: application/json' -d'
{
  "password": "Tz9$mK3pL#qR7xJ@",
  "opendistro_security_roles": [ ],
  "backend_roles": [ ],
  "attributes": { }
}
'
{"status":"CREATED","message":"'testuser' created."}

[INFO ][o.o.s.c.SecurityConfigDiffCalculator] [node1] Detected changes in security configuration: [{"op":"add","path":"/internalusers/testuser","value":{"attributes":{},"backend_roles":[],"hash":"$2y$12$QP.H/ErmdcRh3Azc.55/TO.WdtSAWuKaq.On2ldX/uS8j6LSEkyg6","hidden":false,"opendistro_security_roles":[],"reserved":false,"static":false}}]
[INFO ][o.o.s.c.SecurityConfigVersionHandler] [node1] Successfully saved version v2 to .opensearch_security_config_versions

Fetched users from node 2 :

curl -XGET "https://localhost:9201/_plugins/_security/api/internalusers/?pretty" -u 'admin:admin' --insecure

{
  "logstash" : {
    "hash" : "",
    "reserved" : false,
    "hidden" : false,
    "backend_roles" : [
      "logstash"
    ],
    "attributes" : { },
    "description" : "Demo logstash user, using external role mapping",
    "opendistro_security_roles" : [ ],
    "static" : false
  },
  "snapshotrestore" : {
    "hash" : "",
    "reserved" : false,
    "hidden" : false,
    "backend_roles" : [
      "snapshotrestore"
    ],
    "attributes" : { },
    "description" : "Demo snapshotrestore user, using external role mapping",
    "opendistro_security_roles" : [ ],
    "static" : false
  },
  "admin" : {
    "hash" : "",
    "reserved" : true,
    "hidden" : false,
    "backend_roles" : [
      "admin"
    ],
    "attributes" : { },
    "description" : "Demo admin user",
    "opendistro_security_roles" : [ ],
    "static" : false
  },
  "testuser" : {
    "hash" : "",
    "reserved" : false,
    "hidden" : false,
    "backend_roles" : [ ],
    "attributes" : { },
    "opendistro_security_roles" : [ ],
    "static" : false
  },
  "kibanaserver" : {
    "hash" : "",
    "reserved" : true,
    "hidden" : false,
    "backend_roles" : [ ],
    "attributes" : { },
    "description" : "Demo OpenSearch Dashboards user",
    "opendistro_security_roles" : [ ],
    "static" : false
  },
  "kibanaro" : {
    "hash" : "",
    "reserved" : false,
    "hidden" : false,
    "backend_roles" : [
      "kibanauser",
      "readall"
    ],
    "attributes" : {
      "attribute1" : "value1",
      "attribute2" : "value2",
      "attribute3" : "value3"
    },
    "description" : "Demo OpenSearch Dashboards read only user, using external role mapping",
    "opendistro_security_roles" : [ ],
    "static" : false
  },
  "readall" : {
    "hash" : "",
    "reserved" : false,
    "hidden" : false,
    "backend_roles" : [
      "readall"
    ],
    "attributes" : { },
    "description" : "Demo readall user, using external role mapping",
    "opendistro_security_roles" : [ ],
    "static" : false
  },
  "anomalyadmin" : {
    "hash" : "",
    "reserved" : false,
    "hidden" : false,
    "backend_roles" : [ ],
    "attributes" : { },
    "description" : "Demo anomaly admin user, using internal role",
    "opendistro_security_roles" : [
      "anomaly_full_access"
    ],
    "static" : false
  }
}