- 
                Notifications
    
You must be signed in to change notification settings  - Fork 100
 
AtlassianJira Audit logs #156
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.
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the submission!
Overall it looks like a great reference client to be able to show that communication with the Jira API is functional and fairly straightforward, but there are some core design issues that should be addressed around the query format and limit/offset usage.
I believe this model will cause large queries on the Jira side since no time bounds are in use and should be slightly redesigned using some of the patterns mentioned.
There are lots of examples of time-based queries and bookmarks in other workflows, and if you need some suggestions on where to look please ask.
Thanks again for the submission and I look forward to reviewing again for you.
| @@ -0,0 +1,6 @@ | |||
| <?xml version="1.0" encoding="UTF-8" ?> | |||
| <WorkflowParameterValues xmlns="http://qradar.ibm.com/UniversalCloudRESTAPI/WorkflowParameterValues/V1"> | |||
| <Value name="host" value="" /> | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Readme should include this field along with an example, and should clearly define whether it is just the hostname or the full URL with http://, etc.
I can see from the code it's just the hostname, but providing examples in the readme help usability. Consider including how to specify an alternate port as well if not 443.
| <Parameters> | ||
| <Parameter name="host" label="host" required="true" /> | ||
| <Parameter name="token" label="token" required="true" /> | ||
| <Parameter name="user_id" label="user_id" required="true" /> | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indentation cleanup.
| 
               | 
          ||
| <Initialize path="/get_logs/bookmark" value="0" /> | ||
| <CallEndpoint url="https://${/host}/rest/api/3/auditing/record" method="GET" savePath="/get_logs"> | ||
| <SSLConfiguration allowUntrustedServerCertificate="true" /> | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For security reasons this probably shouldn't be true by default as setting it true can cause extra work.
If this is true, you will HAVE to copy the certificate from the Jira server to the QRadar host. If this server has a proper certificate issued by a recognized issuer you do not need to set this to true and it should be false for a more secure certificate negotiation.
If using a self-signed certificate, then this must be set to true and copy the remote certificate to local.
You may have found during testing that it was needed based on whatever.
I would make this a variable in the Parameters file, and include in the readme when it should be set to true (if using a self-signed certificate) and set it to false by default.
| <Actions> | ||
| 
               | 
          ||
| <Initialize path="/get_logs/bookmark" value="0" /> | ||
| <CallEndpoint url="https://${/host}/rest/api/3/auditing/record" method="GET" savePath="/get_logs"> | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are no query parameters here, meaning no timestamp bounds have been applied, so this query will match EVERY record available EVERY time the workflow runs as it is written here.
I see on line 30 you set the record count as the offset but I don't know if /get_logs/body/total will contain the total number in the query, or just those returned in the one page your query gets.
This will result in a workflow that missed data on the initial result, and then on subsequent runs queries all of time but pages to recent records. This is likely to cause performance issues on the Atlassian side.
Normally a workflow will use a time bound query (from and to) in the Atlassian docs and extract timestamps from each record and persist the last one to be used as the from on subsequent queries.
| 
           Hi,
Thanks for reviewing the commit. Indeed using offset was not the best
option. I have reworked code to use timestamps and it has been running on
our qradar instance for a few weeks now.
Regards,
Ivan
ср, 18 янв. 2023 г. в 15:44, Chris Collins ***@***.***>: 
… ***@***.**** requested changes on this pull request.
 Thanks for the submission!
 Overall it looks like a great reference client to be able to show that
 communication with the Jira API is functional and fairly straightforward,
 but there are some core design issues that should be addressed around the
 query format and limit/offset usage.
 I believe this model will cause large queries on the Jira side since no
 time bounds are in use and should be slightly redesigned using some of the
 patterns mentioned.
 There are lots of examples of time-based queries and bookmarks in other
 workflows, and if you need some suggestions on where to look please ask.
 Thanks again for the submission and I look forward to reviewing again for
 you.
 ------------------------------
 In Community Developed/Atlassian
 Jira/AtlassianJira-Workflow-Parameter-Values.xml
 <#156 (comment)>
 :
 > @@ -0,0 +1,6 @@
 +<?xml version="1.0" encoding="UTF-8" ?>
 +<WorkflowParameterValues xmlns="http://qradar.ibm.com/UniversalCloudRESTAPI/WorkflowParameterValues/V1">
 +    <Value name="host"        value="" />
 The Readme should include this field along with an example, and should
 clearly define whether it is just the hostname or the full URL with http://,
 etc.
 I can see from the code it's just the hostname, but providing examples in
 the readme help usability. Consider including how to specify an alternate
 port as well if not 443.
 ------------------------------
 In Community Developed/Atlassian Jira/AtlassianJira-Workflow.xml
 <#156 (comment)>
 :
 > @@ -0,0 +1,38 @@
 +<?xml version="1.0" encoding="UTF-8" ?>
 +<Workflow name="AtlassianJira" version="1.0" xmlns="http://qradar.ibm.com/UniversalCloudRESTAPI/Workflow/V1">
 +
 +    <Parameters>
 +        <Parameter name="host" label="host" required="true" />
 +        <Parameter name="token" label="token" required="true" />
 +	      <Parameter name="user_id" label="user_id" required="true" />
 Indentation cleanup.
 ------------------------------
 In Community Developed/Atlassian Jira/AtlassianJira-Workflow.xml
 <#156 (comment)>
 :
 > @@ -0,0 +1,38 @@
 +<?xml version="1.0" encoding="UTF-8" ?>
 +<Workflow name="AtlassianJira" version="1.0" xmlns="http://qradar.ibm.com/UniversalCloudRESTAPI/Workflow/V1">
 +
 +    <Parameters>
 +        <Parameter name="host" label="host" required="true" />
 +        <Parameter name="token" label="token" required="true" />
 +	      <Parameter name="user_id" label="user_id" required="true" />
 +    </Parameters>
 +
 +    <Actions>
 +
 +        <Initialize path="/get_logs/bookmark" value="0" />
 +        <CallEndpoint url="https://${/host}/rest/api/3/auditing/record" method="GET" savePath="/get_logs">
 +	    <SSLConfiguration allowUntrustedServerCertificate="true" />
 For security reasons this probably shouldn't be true by default as setting
 it true can cause extra work.
 If this is true, you will HAVE to copy the certificate from the Jira
 server to the QRadar host. If this server has a proper certificate issued
 by a recognized issuer you do not need to set this to true and it should
 be false for a more secure certificate negotiation.
 If using a self-signed certificate, then this must be set to true and
 copy the remote certificate to local.
 You may have found during testing that it was needed based on whatever.
 I would make this a variable in the Parameters file, and include in the
 readme when it should be set to true (if using a self-signed certificate)
 and set it to false by default.
 ------------------------------
 In Community Developed/Atlassian Jira/AtlassianJira-Workflow.xml
 <#156 (comment)>
 :
 > @@ -0,0 +1,38 @@
 +<?xml version="1.0" encoding="UTF-8" ?>
 +<Workflow name="AtlassianJira" version="1.0" xmlns="http://qradar.ibm.com/UniversalCloudRESTAPI/Workflow/V1">
 +
 +    <Parameters>
 +        <Parameter name="host" label="host" required="true" />
 +        <Parameter name="token" label="token" required="true" />
 +	      <Parameter name="user_id" label="user_id" required="true" />
 +    </Parameters>
 +
 +    <Actions>
 +
 +        <Initialize path="/get_logs/bookmark" value="0" />
 +        <CallEndpoint url="https://${/host}/rest/api/3/auditing/record" method="GET" savePath="/get_logs">
 There are no query parameters here, meaning no timestamp bounds have been
 applied, so this query will match EVERY record available EVERY time the
 workflow runs as it is written here.
 https://developer.atlassian.com/cloud/jira/platform/rest/v3/api-group-audit-records/#api-rest-api-3-auditing-record-get
 I see on line 30 you set the record count as the offset but I don't know
 if /get_logs/body/total will contain the total number in the query, or
 just those returned in the one page your query gets.
 This will result in a workflow that missed data on the initial result, and
 then on subsequent runs queries all of time but pages to recent records.
 This is likely to cause performance issues on the Atlassian side.
 Normally a workflow will use a time bound query (from and to) in the
 Atlassian docs and extract timestamps from each record and persist the last
 one to be used as the from on subsequent queries.
 —
 Reply to this email directly, view it on GitHub
 <#156 (review)>,
 or unsubscribe
 <https://github.com/notifications/unsubscribe-auth/ASMJDNH3N3BVLHBO7W4PXV3WS76TFANCNFSM6AAAAAAT643HRU>
 .
 You are receiving this because you authored the thread.Message ID:
 ***@***.***
 .com>
 
-- 
С уважением,
Иван Синяков. 
 | 
    
| 
           Still awaiting required changes on this PR before merge if you're still interested in contributing. Thanks!  | 
    
No description provided.