feat: emit kernel and os metrics with telemetry#1716
Merged
alter-mage merged 1 commit intomainfrom Jun 23, 2025
Merged
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| List<String> installedKnowledgeBaseArticles = new ArrayList<>(); | ||
| String knowledgeBaseQueryCommand = "Get-HotFix | Select-Object -ExpandProperty HotFixID"; | ||
| try { | ||
| ProcessBuilder builder = new ProcessBuilder("powershell.exe", |
Check warning
Code scanning / CodeQL
Executing a command with a relative path Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 9 months ago
To fix the issue, replace the relative path "powershell.exe" with its absolute path. On most Windows systems, the absolute path to PowerShell is "C:\\Windows\\System32\\WindowsPowerShell\\v1.0\\powershell.exe". This ensures that the correct executable is used regardless of the PATH environment variable.
The changes will be applied to both instances where "powershell.exe" is used in the WindowsPlatform class:
- Line 708 in the
getInstalledKBsmethod. - Line 731 in the
executePowerShellCommandmethod.
Suggested changeset
1
src/main/java/com/aws/greengrass/util/platforms/windows/WindowsPlatform.java
| @@ -707,3 +707,3 @@ | ||
| try { | ||
| ProcessBuilder builder = new ProcessBuilder("powershell.exe", | ||
| ProcessBuilder builder = new ProcessBuilder("C:\\Windows\\System32\\WindowsPowerShell\\v1.0\\powershell.exe", | ||
| "-Command", knowledgeBaseQueryCommand); | ||
| @@ -730,3 +730,3 @@ | ||
| try { | ||
| ProcessBuilder builder = new ProcessBuilder("powershell.exe", "-Command", command); | ||
| ProcessBuilder builder = new ProcessBuilder("C:\\Windows\\System32\\WindowsPowerShell\\v1.0\\powershell.exe", "-Command", command); | ||
| Process process = builder.start(); |
Copilot is powered by AI and may make mistakes. Always verify output.
src/main/java/com/aws/greengrass/util/platforms/windows/WindowsPlatform.java
Fixed
Show fixed
Hide fixed
|
|
||
| private String executePowerShellCommand(String command) { | ||
| try { | ||
| ProcessBuilder builder = new ProcessBuilder("powershell.exe", "-Command", command); |
Check warning
Code scanning / CodeQL
Executing a command with a relative path Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 9 months ago
To fix the issue, replace the relative path "powershell.exe" with its absolute path. On most Windows systems, the absolute path to PowerShell is C:\\Windows\\System32\\WindowsPowerShell\\v1.0\\powershell.exe. This ensures that the correct executable is always used, regardless of the PATH environment variable.
The changes will involve:
- Defining a constant for the absolute path to
powershell.exeto avoid hardcoding it multiple times. - Updating all instances where
"powershell.exe"is used inProcessBuilderto use the absolute path constant.
Suggested changeset
1
src/main/java/com/aws/greengrass/util/platforms/windows/WindowsPlatform.java
| @@ -80,2 +80,3 @@ | ||
| public class WindowsPlatform extends Platform { | ||
| private static final String POWERSHELL_ABSOLUTE_PATH = "C:\\Windows\\System32\\WindowsPowerShell\\v1.0\\powershell.exe"; | ||
| public static final String LOADER_LOGS_FILE_NAME = "greengrass.out.log"; | ||
| @@ -707,3 +708,3 @@ | ||
| try { | ||
| ProcessBuilder builder = new ProcessBuilder("powershell.exe", | ||
| ProcessBuilder builder = new ProcessBuilder(POWERSHELL_ABSOLUTE_PATH, | ||
| "-Command", knowledgeBaseQueryCommand); | ||
| @@ -730,3 +731,3 @@ | ||
| try { | ||
| ProcessBuilder builder = new ProcessBuilder("powershell.exe", "-Command", command); | ||
| ProcessBuilder builder = new ProcessBuilder(POWERSHELL_ABSOLUTE_PATH, "-Command", command); | ||
| Process process = builder.start(); |
Copilot is powered by AI and may make mistakes. Always verify output.
src/main/java/com/aws/greengrass/util/platforms/windows/WindowsPlatform.java
Fixed
Show fixed
Hide fixed
MikeDombo
reviewed
Jun 12, 2025
| ProcessBuilder builder = new ProcessBuilder("powershell.exe", | ||
| "-Command", knowledgeBaseQueryCommand); | ||
| Process process = builder.start(); | ||
| BufferedReader reader = new BufferedReader(new InputStreamReader(process.getInputStream())); |
Member
There was a problem hiding this comment.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
InputStreamReader
While converting a byte stream into characters, if no encoding is specified, the constructor assumes the default system runtime encoding, which may not be the correct encoding.
Suggested solution:
Specify an encoding
(likely UTF-8, which is backward compatible with ASCII and has multi-language support)
InputStreamReader reader = new InputStreamReader(inputStream, StandardCharsets.UTF_8);
| try { | ||
| ProcessBuilder builder = new ProcessBuilder("powershell.exe", "-Command", command); | ||
| Process process = builder.start(); | ||
| BufferedReader reader = new BufferedReader(new InputStreamReader(process.getInputStream())); |
Member
There was a problem hiding this comment.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
InputStreamReader
While converting a byte stream into characters, if no encoding is specified, the constructor assumes the default system runtime encoding, which may not be the correct encoding.
Suggested solution:
Specify an encoding
(likely UTF-8, which is backward compatible with ASCII and has multi-language support)
InputStreamReader reader = new InputStreamReader(inputStream, StandardCharsets.UTF_8);
| Process process = builder.start(); | ||
| BufferedReader reader = new BufferedReader(new InputStreamReader(process.getInputStream())); | ||
| String line; | ||
| while ((line = reader.readLine()) != null) { |
Member
There was a problem hiding this comment.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
An attacker can provide an input stream or file that contains an excessive number of lines without a terminating condition. The call to readLine() might allow an attacker to crash the program or otherwise make it unavailable to legitimate users which causes denial of service. To avoid this, make sure you either enforce a limit on the lines read or apply proper input sanitization.
| ProcessBuilder builder = new ProcessBuilder("powershell.exe", "-Command", command); | ||
| Process process = builder.start(); | ||
| BufferedReader reader = new BufferedReader(new InputStreamReader(process.getInputStream())); | ||
| String output = reader.readLine(); |
Member
There was a problem hiding this comment.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
An attacker can provide an input stream or file that contains an excessive number of lines without a terminating condition. The call to readLine() might allow an attacker to crash the program or otherwise make it unavailable to legitimate users which causes denial of service. To avoid this, make sure you either enforce a limit on the lines read or apply proper input sanitization.
| try { | ||
| ProcessBuilder builder = new ProcessBuilder("powershell.exe", "-Command", command); | ||
| Process process = builder.start(); | ||
| BufferedReader reader = new BufferedReader(new InputStreamReader(process.getInputStream())); |
Member
There was a problem hiding this comment.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
The code is not properly closing resources, which can lead to resource leaks. Unclosed resources may cause memory leaks or other system resource issues, potentially degrading application performance over time. If an exception occurs before the explicit close() call, the resource will not be closed, increasing the risk of leaks. To remediate this, use the try-with-resources statement to automatically close AutoCloseable resources, ensuring proper resource management even when exceptions occur.
| ProcessBuilder builder = new ProcessBuilder("powershell.exe", | ||
| "-Command", knowledgeBaseQueryCommand); | ||
| Process process = builder.start(); | ||
| BufferedReader reader = new BufferedReader(new InputStreamReader(process.getInputStream())); |
Member
There was a problem hiding this comment.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
The code is not properly closing resources, which can lead to resource leaks. Unclosed resources may cause memory leaks or other system resource issues, potentially degrading application performance over time. If an exception occurs before the explicit close() call, the resource will not be closed, increasing the risk of leaks. To remediate this, use the try-with-resources statement to automatically close AutoCloseable resources, ensuring proper resource management even when exceptions occur.
|
Binary incompatibility detected for commit 2f43c0e. com.aws.greengrass.lifecyclemanager.KernelAlternatives is binary incompatible and is source incompatible because of METHOD_LESS_ACCESSIBLE Produced by binaryCompatability.py |
|
Unit Tests Coverage Report
Minimum allowed coverage is Generated by 🐒 cobertura-action against 2f43c0e |
|
Integration Tests Coverage Report
Minimum allowed coverage is Generated by 🐒 cobertura-action against 2f43c0e |
robcmann
reviewed
Jun 16, 2025
| // If there are no metrics to be published, then we should return and not publish any telemetry messages. | ||
| if (aggUploadMetrics.isEmpty()) { | ||
| return aggUploadMetrics; | ||
| if (!aggUploadMetrics.isEmpty()) { |
Contributor
There was a problem hiding this comment.
Should we still be returning fast if aggUploadMetrics is empty?
Contributor
There was a problem hiding this comment.
Or is the thought here that we will always add the kernel and OS metrics
Member
Author
There was a problem hiding this comment.
Yes, the latter. It is possible that we publish before any other metrics are even aggregated. In any case, we should atleast transmit the OS and kernel metrics.
robcmann
reviewed
Jun 17, 2025
| return ""; | ||
| } | ||
|
|
||
| // Match patterns like: |
Contributor
There was a problem hiding this comment.
Is this pattern universal across all Linux operating systems we support?
Member
Author
There was a problem hiding this comment.
That's a good question. Further research suggests that the answer is no and it looks like a regex which can support all unix distributions does not exist. I will push back on this method.
robcmann
reviewed
Jun 17, 2025
robcmann
approved these changes
Jun 19, 2025
saranyailla
approved these changes
Jun 23, 2025
Member
saranyailla
left a comment
saranyailla
left a comment
There was a problem hiding this comment.
Approving this PR, but noting some points for future consideration:
- These metrics don't require daily aggregation.
- While units are standard, we could add custom unit types to better support these metrics (as discussed internally)
- We could explore additional aggregation types beyond
Sum, both on cloud/device side (as discussed internally)
These are not blocking concerns but worth keeping in mind for future iterations.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue #, if available:
Description of changes:
Emit OS and kernel metrics as part of the telemetry messages. A list of OS and Kernel metrics is as follows
A sample entry on an Amazon Linux 2 looks like
Why is this change necessary:
Telemetry should include kernel and OS related metrics such that we and our customers can create E2E workflows to query devices running a particular OS or kernel. This helps us to locate Greengrass running on devices with, for example, a vulnerable OS or kernel.
How was this change tested:
Any additional information or context required to review the change:
Documentation Checklist:
Compatibility Checklist:
any deprecated method or type.
Refer to Compatibility Guidelines for more information.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.