Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Store SIA user agent information in x509 certificate request table #2772

Merged
merged 7 commits into from
Oct 24, 2024
Merged

Store SIA user agent information in x509 certificate request table #2772

merged 7 commits into from
Oct 24, 2024

Conversation

rajeshal
Copy link
Contributor

@rajeshal rajeshal commented Oct 18, 2024 •
edited
Loading

Description

Contribution Checklist:

  • The pull request does not introduce any breaking changes
  • I have read the contribution guidelines.
  • Create an issue and link to the pull request.

Attach Screenshots (Optional)

@rajeshal rajeshal force-pushed the index_sia_agent_reference branch 2 times, most recently from c251cba to 2eb5962 Compare October 18, 2024 18:49
havetisyan
havetisyan requested changes Oct 22, 2024
View reviewed changes
...on/src/main/java/com/yahoo/athenz/common/server/cert/impl/JDBCCertRecordStoreConnection.java Outdated
@@ -208,6 +210,7 @@ public boolean updateX509CertRecord(X509CertRecord certRecord) throws ServerReso
ps.setString(10, certRecord.getProvider());
ps.setString(11, certRecord.getInstanceId());
ps.setString(12, certRecord.getService());
ps.setString(13, certRecord.getSiaProvider());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the sia provider is not the last component in the UPDATE command. It should be field 10 and then others are pushed back by 1.

can you also please review the test case for this method since this should have been caught by the unit tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, good catch.

servers/zts/schema/zts_server.sql Outdated
@@ -35,6 +35,7 @@ CREATE TABLE IF NOT EXISTS `zts_store`.`certificates` (
`lastNotifiedServer` VARCHAR(512) NULL,
`expiryTime` DATETIME(3) NULL,
`hostName` VARCHAR(512) NULL,
`siaProvider` VARCHAR(256) NOT NULL,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are the instructions for the ZMS schema but they apply to ZTS as well:

https://github.com/AthenZ/athenz/blob/master/servers/zms/schema/schema_update.md

@rajeshal rajeshal force-pushed the index_sia_agent_reference branch 3 times, most recently from a62daac to 8a90860 Compare October 22, 2024 16:53
havetisyan
havetisyan reviewed Oct 23, 2024
View reviewed changes
...java/server_common/src/main/java/com/yahoo/athenz/common/server/util/ServletRequestUtil.java Outdated
* @param request http servlet request
* @return SIA agent along with version, like 'SIA-FARGATE 1.32.0'
**/
public static String getSiaAgent(final HttpServletRequest request) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there any reason why we want to maintain the full value of the agent including the version number? Why not just extract the value after SIA- and then our provider will be EKS, AWS, Fargate, etc so it's easier to index and just run queries. Otherwise you'll have SIA-EKS 1.52.0, SIA-EKS 1.53.0, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the provider agent version it will give more flexibility in general to filter based on certain agent version. Since the version is at the end queries can still use fuzzy search in indexes.
Also will be scalable if another type of Service Identity Agent come into picture in addition to SIA

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If that is the case, then the field should be called userAgent and not siaProvider since the user agent is not provider. Not sure exactly what use cases you're referring to regarding the version number - that information is already in the access log file so no need to maintain it here. In fact, for a lot of lambda identities, we're gong to get something like 'Jersey/2.35 (Apache HttpClient 4.5.13)' so that's not useful at all.

We should keep our data clean and only save what we absolutely need. So we should get the agent, parse and only take agents with the format "SIA- ", extract the provider and save that value only. For all other cases, the provider will be empty and that should be excluded from any processing.

servers/zts/schema/zts_server.sql
PRIMARY KEY (`provider`, `instanceId`, `service`),
INDEX `idx_hostName` (`hostName` ASC))
INDEX `idx_hostName` (`hostName` ASC) VISIBLE)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how did we get the VISIBLE flag here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is mandatory for current version of MySQL Workbench

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So who ever touch a table via MySQL Workbench, it change the index with default value VISIBLE.
Since that is the default value, it should be fine as I don't think Athenz uses any indexes not to be used by optimizer.

rajeshal added 5 commits October 23, 2024 18:19
Store SIA user agent information in x509 certificate request table
b5976f4 
Signed-off-by: rajeshal <rajeshal@yahooinc.com>
fix test case
4db40d0 
Signed-off-by: rajeshal <rajeshal@yahooinc.com>
review changes
ca80b90 
Signed-off-by: rajeshal <rajeshal@yahooinc.com>
add missing fields in zts_server.mwb for workload table
4de4e2e 
Signed-off-by: rajeshal <rajeshal@yahooinc.com>
Fix test cases
4127c74 
Signed-off-by: rajeshal <rajeshal@yahooinc.com>
extract SIA provider name from user agent
45a0e63 
Signed-off-by: rajeshal <rajeshal@yahooinc.com>
havetisyan
havetisyan reviewed Oct 24, 2024
View reviewed changes
...java/server_common/src/test/java/com/yahoo/athenz/common/server/cert/X509CertRecordTest.java Outdated
@@ -45,6 +45,7 @@ public void testX509CertRecord() {
certRecord.setLastNotifiedTime(now);
certRecord.setExpiryTime(now);
certRecord.setSvcDataUpdateTime(now);
certRecord.setSiaProvider("SIA-EKS 1.52.0");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's update this test case to use cert.setSiaProvider("EKS") to be consistent with the implementation.

fix test case value
44020b9 
Signed-off-by: rajeshal <rajeshal@yahooinc.com>
@havetisyan havetisyan merged commit 483e0ab into AthenZ:master Oct 24, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@havetisyan havetisyan havetisyan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rajeshal @havetisyan