V2.2+tc pvt #1
V2.2+tc pvt #1
Conversation
…ool' of ssh://git-amr-2.devtools.intel.com:29418/dcg_security-mtwilson into v2.1.0+feature#5600-bkc-tool
…in mtwilson.env to allow local api access for validation script using curl
…CIT BKC Tool installer for generating random passwords
…ls.intel.com:29418/dcg_security-mtwilson into v2.1.0+feature#5600-bkc-tool
…n.sh being excluded
…ool' of ssh://git-amr-2.devtools.intel.com:29418/dcg_security-mtwilson into v2.1.0+feature#5600-bkc-tool
…n to functions.sh added localhost-integration call to tagent.sh added service restart after localhost-integration call
…ethod in functions.sh
…nable localhost access to apis via shiro authentication
…n and asset tag provisioning APIs
…ool' of ssh://git-amr-2.devtools.intel.com:29418/dcg_security-mtwilson into v2.1.0+feature#5600-bkc-tool
…changed to move shiro-localhost.ini file to shiro.ini file location
…ool' of ssh://git-amr-2.devtools.intel.com:29418/dcg_security-mtwilson into v2.1.0+feature#5600-bkc-tool
…lias which forces prompt commented out "else" statement to prevent syntax error
…ddressed the code review feedback from Ryan and also updated the script to use xmlstarlet instead of sed.
…cause functions.sh is in common-java and this copy was breaking the build
…nd escaped "cp" alias
…ool' of ssh://git-amr-2.devtools.intel.com:29418/dcg_security-mtwilson into v2.1.0+feature#5600-bkc-tool # Conflicts: # installers/LinuxUtil/src/files/functions.sh
…ml as it was deleted/moved to common-java
…5600-bkc-tool # Conflicts: # installers/LinuxUtil/pom.xml (removed: functions.sh is in common-java, mtwilson.sh is now in mtwilson-server) # installers/mtwilson-trustagent/src/files/setup_prereqs.sh (merged) # trust-agent/mtwilson-trustagent-setup/src/main/java/com/intel/mtwilson/trustagent/setup/CreateTpmOwnerSecret.java (merged)
…ool' of ssh://git-amr-2.devtools.intel.com:29418/dcg_security-mtwilson into v2.1.0+feature#5600-bkc-tool
…' into v2.1.0+feature#5600-bkc-tool # Conflicts: # trust-agent/mtwilson-trustagent-console/src/main/java/com/intel/mtwilson/trustagent/cmd/StartHttpServer.java
…bkc-tool' into dev-cit-2.2+feature#5600-bkc-tool
…v-cit-2.2+feature#5600-bkc-tool
…v-cit-2.2+feature#5600-bkc-tool
…v-cit-2.2+feature#5600-bkc-tool Conflicts: installers/LinuxUtil/src/files/functions.sh
…5600-bkc-tool' of ssh://git-amr-2.devtools.intel.com:29418/dcg_security-mtwilson into dev-cit-2.2+feature#5600-bkc-tool
…c-tool' into v2.2+feature#5600-bkc-tool
|
Unable to locate .performanceTestingBot config file |
|
Seems you are using me but didn't get OPENAI_API_KEY seted in Variables/Secrets for this repo. you could follow readme for more information |
|
Thanks @2lambda123 for opening this PR! For COLLABORATOR only :
|
|
Processing PR updates... |
![gitginie[bot]](https://avatars.githubusercontent.com/in/712912?s=60&v=4){kind=small}
There was a problem hiding this comment.
@2lambda123
Thank you for your contribution to this repository! We appreciate your effort in opening pull request.
Happy coding!
Their most recently public accepted PR is: 2lambda123/crypto-com-curve25519-dalek#4 |
|
Your organization has reached the subscribed usage limit. You can upgrade your account by purchasing a subscription at Stripe payment link Disclaimer: This comment was entirely generated using AI. Be aware that the information provided may be incorrect. Current plan usage: 117.00% Have feedback or need help? |
|
First PR by @2lambda123 PR Details of @2lambda123 in opencit-opencit :
|
Reviewer's Guide by SourceryThis pull request implements several changes to improve the installation and setup process for the CIT (Cloud Integrity Technology) components, including the Trust Agent and Attestation Service. The changes focus on streamlining the installation process, improving error handling, and adding support for TPM 2.0. File-Level Changes
Tips
|
There was a problem hiding this comment.
@2lambda123
Thank you for your contribution to this repository! We appreciate your effort in closing pull request.
Happy coding!
|
Warning Rate limit exceeded@labels-and-badges[bot] has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 20 minutes and 12 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes introduced in this update enhance the functionality, error handling, and dependency management of multiple components within the MT Wilson project ecosystem. Key updates involve modifications to installation scripts, more robust configuration management, and improved integration processes for security features. These enhancements collectively aim to bolster the overall robustness and user experience of the system, ensuring smoother operations for the Attestation service and related tools. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Installer
participant Configuration
participant Service
User->>Installer: Initiate installation
Installer->>Configuration: Load configuration
Configuration-->>Installer: Config values
Installer->>Service: Start service
Service-->>Installer: Service started
Installer-->>User: Installation complete
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
labels-and-badges
bot
added
NO JIRA
![codereviewbot-ai[bot]](https://avatars.githubusercontent.com/in/309638?s=60&v=4){kind=small}
| auto_install "Developer tools" "DEVELOPER" >> "$INSTALL_LOG_FILE" | ||
| if [ $? -ne 0 ]; then echo_failure "Failed to install prerequisites through package installer"; exit -1; fi |
There was a problem hiding this comment.
The use of exit -1 is not standard for indicating failure in shell scripts. The standard exit code for general errors is 1. Using -1 can lead to confusion and is not portable across different systems.
Recommended Solution: Replace exit -1 with exit 1 to follow standard conventions for exit codes.
| localhost-integration) | ||
| mtwilson_localhost_integration $@ | ||
| #mtwilson_localhost_integration $@ | ||
| shiro_localhost_integration "/opt/mtwilson/configuration/shiro.ini" | ||
| /opt/mtwilson/bin/mtwilson.sh restart |
There was a problem hiding this comment.
The localhost-integration case calls shiro_localhost_integration and then immediately restarts the service using /opt/mtwilson/bin/mtwilson.sh restart. If shiro_localhost_integration fails for any reason, the script will still proceed to restart the service, which could lead to inconsistent states or failures.
Recommendation: Add error handling to check the exit status of shiro_localhost_integration before proceeding to restart the service. If the command fails, the script should exit or handle the error appropriately.
| <!-- | ||
| <artifactItem> | ||
| <groupId>net.sourceforge.tboot</groupId> | ||
| <artifactId>tboot</artifactId> | ||
| <version>1.9.4</version> | ||
| <classifier>x86_64</classifier> | ||
| <type>rpm</type> | ||
| <outputDirectory>${makeself.directory}</outputDirectory> | ||
| </artifactItem> | ||
| <artifactItem> | ||
| <groupId>net.sourceforge.tboot</groupId> | ||
| <artifactId>tboot</artifactId> | ||
| <version>1.9.4</version> | ||
| <classifier>amd64</classifier> | ||
| <type>deb</type> | ||
| <outputDirectory>${makeself.directory}</outputDirectory> | ||
| </artifactItem> | ||
| --> |
There was a problem hiding this comment.
Commented-out code blocks can lead to confusion and clutter in the codebase. If the commented-out dependencies are no longer needed, they should be removed. If they might be needed in the future, consider documenting the reason for their presence and any conditions under which they should be uncommented.
Recommended Solution: Remove the commented-out code if it is no longer necessary, or add a comment explaining why it is kept and under what conditions it should be used.
| <!-- | ||
| <dependency> | ||
| <groupId>net.sourceforge.tboot</groupId> | ||
| <artifactId>tboot</artifactId> | ||
| <version>1.9.4</version> | ||
| <classifier>x86_64</classifier> | ||
| <type>rpm</type> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>net.sourceforge.tboot</groupId> | ||
| <artifactId>tboot</artifactId> | ||
| <version>1.9.4</version> | ||
| <classifier>amd64</classifier> | ||
| <type>deb</type> | ||
| </dependency> | ||
| --> |
There was a problem hiding this comment.
Commented-out dependencies for tboot are present in the POM file. If these dependencies are no longer needed, they should be removed to keep the POM file clean and maintainable. If they might be needed in the future, consider documenting the reason for keeping them commented out.
| <!-- | ||
| <artifactItem> | ||
| <groupId>net.sourceforge.tboot</groupId> | ||
| <artifactId>tboot</artifactId> | ||
| <version>1.9.4</version> | ||
| <classifier>x86_64</classifier> | ||
| <type>rpm</type> | ||
| <outputDirectory>${makeself.directory}</outputDirectory> | ||
| </artifactItem> | ||
| <artifactItem> | ||
| <groupId>net.sourceforge.tboot</groupId> | ||
| <artifactId>tboot</artifactId> | ||
| <version>1.9.4</version> | ||
| <classifier>amd64</classifier> | ||
| <type>deb</type> | ||
| <outputDirectory>${makeself.directory}</outputDirectory> | ||
| </artifactItem> | ||
| --> |
There was a problem hiding this comment.
Commented-out artifact items for tboot are present in the POM file. If these artifact items are no longer needed, they should be removed to keep the POM file clean and maintainable. If they might be needed in the future, consider documenting the reason for keeping them commented out.
| if [ $result -eq 255 ] || [ -f "/opt/mtwilson/var/reboot_required" ]; then | ||
| rm -f /opt/mtwilson/var/reboot_required | ||
| echo_info "CIT Attestation Service requires reboot to continue"; | ||
| exit 255 |
There was a problem hiding this comment.
The script exits with code 255 if a reboot is required, but it does not provide any mechanism to resume the installation after the reboot. This could lead to incomplete installations if the user does not manually restart the script.
Recommended Solution: Implement a mechanism to resume the installation automatically after a reboot, such as creating a flag file that the script checks for and continues from where it left off.
| MTWILSON_ADMIN_PASSWORD= | ||
| MTWILSON_DATABASE_PASSWORD= | ||
| MTWILSON_PRIVACYCA_PASSWORD= | ||
| MTWILSON_TAG_ADMIN_PASSWORD= | ||
| MTWILSON_TAG_XML_PASSWORD= |
There was a problem hiding this comment.
Sensitive information such as passwords (MTWILSON_ADMIN_PASSWORD, MTWILSON_DATABASE_PASSWORD, MTWILSON_PRIVACYCA_PASSWORD, MTWILSON_TAG_ADMIN_PASSWORD, MTWILSON_TAG_XML_PASSWORD) are being stored in environment variables. This can be a security risk as environment variables can be accessed by any process running with the same user privileges.
Recommendation: Use a secure secrets management service or tool to handle sensitive information instead of storing them in environment variables. If environment variables must be used, ensure they are set in a secure manner and access is restricted.
| # 2. run the "install.sh" script from the destination folder | ||
| # that script will run subordinate installers and automatically resume after reboot | ||
|
|
||
| source ./cit-bkc-tool.env |
There was a problem hiding this comment.
Using source in a script that starts with #!/bin/sh can lead to compatibility issues because source is not POSIX-compliant and may not be available in all shell environments. Instead, use the POSIX-compliant . (dot) command to source the file.
| rm -rf $CIT_BKC_PACKAGE_PATH | ||
| mkdir -p $CIT_BKC_PACKAGE_PATH | ||
| cp * $CIT_BKC_PACKAGE_PATH |
There was a problem hiding this comment.
The script uses rm -rf and cp * without proper validation or quoting, which can lead to unintended deletions or copying of files, especially if filenames contain spaces or special characters. Always quote variables and consider adding checks to ensure the paths and filenames are safe.
| MTWILSON_TLS_CERT_SHA1= | ||
| MTWILSON_TLS_CERT_SHA256=<MTWILSON_TLS_CERT_SHA256> | ||
| MTWILSON_API_USERNAME=pca-admin | ||
| MTWILSON_API_PASSWORD=password |
There was a problem hiding this comment.
Storing sensitive information such as MTWILSON_API_PASSWORD directly in the environment file is a security risk. This can lead to unauthorized access if the file is exposed.
Recommended Solution: Use a secure secrets management service or environment variable injection mechanism to handle sensitive information. Ensure that the password is not hardcoded and is securely retrieved at runtime.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @2lambda123 - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- Potential hard-coded secret: admin password generation. (link)
- Potential hard-coded secret: database password generation. (link)
- Potential hard-coded secret: Privacy CA password generation. (link)
- Potential hard-coded secret: tag admin password generation. (link)
- Potential hard-coded secret: tag XML password generation. (link)
- Potential hard-coded secret: API password update. (link)
Overall Comments:
- This is a substantial refactoring that improves modularity and TPM 2.0 support. Please ensure thorough testing has been done on both TPM 1.2 and 2.0 systems, as well as different Linux distributions.
- The new CIT BKC Tool looks like a valuable addition for validating deployments. Consider adding some documentation or user guide for this new tool.
- The removal of the LinuxUtil scripts represents a significant change. Make sure all functionality has been properly migrated to the new scripts and nothing important was lost in the transition.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🔴 Security: 6 blocking issues, 2 other issues
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟡 Documentation: 1 issue found
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| } | ||
|
|
||
| cit_bkc_print_env() { | ||
| env | grep -E "^CIT_BKC|^http_proxy|^https_proxy" |
There was a problem hiding this comment.
🚨 suggestion (security): Consider safer alternative to 'eval' for environment variables
The use of 'eval' to load environment variables can be risky. Consider using a safer method to parse and set environment variables.
| env | grep -E "^CIT_BKC|^http_proxy|^https_proxy" | |
| cit_bkc_print_env() { | |
| printenv | grep -E "^(CIT_BKC|http_proxy|https_proxy)=" | |
| } |
| # identify tpm version | ||
| # postcondition: | ||
| # variable TPM_VERSION is set to 1.2 or 2.0 | ||
| detect_tpm_version() { |
There was a problem hiding this comment.
suggestion: Consider using a more robust method for TPM version detection
The current method relies on file checks, which might not be reliable across all systems. Consider using system commands or libraries specifically designed for TPM detection to ensure accuracy.
detect_tpm_version() {
export TPM_VERSION
if command -v tpm2_getcap >/dev/null 2>&1; then
TPM_VERSION="2.0"
elif command -v tpm_version >/dev/null 2>&1; then
TPM_VERSION="1.2"
else
echo "Unable to detect TPM version" >&2
return 1
fi
}
| if [ -z "$target" ] || [ ! -f "$target" ]; then return 1; fi | ||
| source "$target" | ||
| local env_file_exports=$(cat "$target" | grep -E '^[A-Z0-9_]+\s*=' | cut -d = -f 1) | ||
| if [ -n "$env_file_exports" ]; then eval export $env_file_exports; fi |
There was a problem hiding this comment.
🚨 issue (security): Replace eval with a safer alternative for exporting variables
Using eval to export variables from an environment file is a potential security risk if the file contains malicious content. Consider using a safer approach, such as parsing the file line by line and exporting variables individually.
| chmod +x *.sh *.bin | ||
|
|
||
| pwd=$(pwd) | ||
| if [ "$pwd" != "$CIT_BKC_PACKAGE_PATH" ]; then |
There was a problem hiding this comment.
suggestion: Avoid hardcoded paths and allow for configuration
The script uses hardcoded paths like $CIT_BKC_PACKAGE_PATH. Consider allowing these to be configured or use relative paths where possible to improve flexibility and maintainability.
| if [ "$pwd" != "$CIT_BKC_PACKAGE_PATH" ]; then | |
| if [ "$pwd" != "${CIT_BKC_PACKAGE_PATH:-$HOME/cit_bkc_package}" ]; then | |
| CIT_BKC_PACKAGE_PATH="${CIT_BKC_PACKAGE_PATH:-$HOME/cit_bkc_package}" |
| This tool tests the platform hardware and software stack to validate that | ||
| it is ready for Intel(R) Cloud Integrity Technology. |
There was a problem hiding this comment.
suggestion (documentation): Consider consistent capitalization for product name
Ensure 'Intel(R) Cloud Integrity Technology' is capitalized consistently throughout the document. The first mention uses all caps, while this one doesn't.
| cp $CIT_BKC_PACKAGE_PATH/mtwilson.env $HOME/mtwilson.env | ||
| local admin_passwd=$(generate_password 16) | ||
| update_property_in_file MTWILSON_ADMIN_PASSWORD $HOME/mtwilson.env "$admin_passwd" | ||
| local database_passwd=$(generate_password 16) |
There was a problem hiding this comment.
🚨 issue (security): Potential hard-coded secret: database password generation.
Ensure that the generated database password is securely stored and not exposed in logs or error messages.
| update_property_in_file MTWILSON_ADMIN_PASSWORD $HOME/mtwilson.env "$admin_passwd" | ||
| local database_passwd=$(generate_password 16) | ||
| update_property_in_file MTWILSON_DATABASE_PASSWORD $HOME/mtwilson.env "$database_passwd" | ||
| local pca_passwd=$(generate_password 16) |
There was a problem hiding this comment.
🚨 issue (security): Potential hard-coded secret: Privacy CA password generation.
Ensure that the generated Privacy CA password is securely stored and not exposed in logs or error messages.
| update_property_in_file MTWILSON_DATABASE_PASSWORD $HOME/mtwilson.env "$database_passwd" | ||
| local pca_passwd=$(generate_password 16) | ||
| update_property_in_file MTWILSON_PRIVACYCA_PASSWORD $HOME/mtwilson.env "$pca_passwd" | ||
| local tag_passwd=$(generate_password 16) |
There was a problem hiding this comment.
🚨 issue (security): Potential hard-coded secret: tag admin password generation.
Ensure that the generated tag admin password is securely stored and not exposed in logs or error messages.
| update_property_in_file MTWILSON_PRIVACYCA_PASSWORD $HOME/mtwilson.env "$pca_passwd" | ||
| local tag_passwd=$(generate_password 16) | ||
| update_property_in_file MTWILSON_TAG_ADMIN_PASSWORD $HOME/mtwilson.env "$tag_passwd" | ||
| local tagxml_passwd=$(generate_password 16) |
There was a problem hiding this comment.
🚨 issue (security): Potential hard-coded secret: tag XML password generation.
Ensure that the generated tag XML password is securely stored and not exposed in logs or error messages.
| update_property_in_file MTWILSON_TLS_CERT_SHA1 $HOME/trustagent.env "$tls_sha1" | ||
| update_property_in_file MTWILSON_TLS_CERT_SHA256 $HOME/trustagent.env "$tls_sha256" | ||
| pca_passwd=$(read_property_from_file MTWILSON_PRIVACYCA_PASSWORD $HOME/mtwilson.env) | ||
| update_property_in_file MTWILSON_API_PASSWORD $HOME/trustagent.env "$pca_passwd" |
There was a problem hiding this comment.
🚨 issue (security): Potential hard-coded secret: API password update.
Ensure that the API password is securely stored and not exposed in logs or error messages.
Description
Related Issue
Types of changes
Checklist:
Summary by Sourcery
Refactor and enhance the setup scripts for Trust Agent and Attestation Service, introducing new scripts for CIT BKC validation and installation. Improve TPM version handling and modularize setup functions. Remove deprecated scripts to streamline the installation process.
New Features:
Enhancements:
Chores:
Summary by CodeRabbit
New Features
cit-bkc-tool, a command-line utility for Intel Cloud Integrity Technology, providing various functionalities including validation and installation management.Bug Fixes
Documentation
README.mdforcit-bkc-tooloutlining installation and usage instructions.