Skip to content

aws: add pugixml dependency#30973

Closed
suniltheta wants to merge 2 commits intoenvoyproxy:mainfrom
suniltheta:pugixml
Closed

aws: add pugixml dependency#30973
suniltheta wants to merge 2 commits intoenvoyproxy:mainfrom
suniltheta:pugixml

Conversation

@suniltheta
Copy link
Copy Markdown
Contributor

Commit Message: aws: add pugixml dependency
Additional Description: We would like to introduce pugixml dependency into Envoy to parse the xml content. The example usecase is shown in one of the unmerged PR 23408 (note: tinyxml show in code will be replaced with pugixml). This external library let's us parse the AssumeRoleWithWebIdentityResponse provided by AWS STS service https://docs.aws.amazon.com/STS/latest/APIReference/API_AssumeRoleWithWebIdentity.html#API_AssumeRoleWithWebIdentity_Examples to fetch the AWS credentials for some of the AWS extensions such as AWS Lambda filter, AWS Request Signer filter etc.
Risk Level: -
Testing: NA
Docs Changes: NA
Release Notes: NA
Platform Specific Features: NA


The PR adds a new dependency on https://github.com/zeux/pugixml.

OpenSSF Scorecard

RESULTS
-------
Aggregate score: 5.5 / 10

Check scores:
|---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------|
|  SCORE  |          NAME          |             REASON             |                                               DOCUMENTATION/REMEDIATION                                               |
|---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------|
| 10 / 10 | Binary-Artifacts       | no binaries found in the repo  | https://github.com/ossf/scorecard/blob/70c8e05d6dca69defac0fee1edbbf3fa8732af00/docs/checks.md#binary-artifacts       |
|---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------|
| 3 / 10  | Branch-Protection      | branch protection is not       | https://github.com/ossf/scorecard/blob/70c8e05d6dca69defac0fee1edbbf3fa8732af00/docs/checks.md#branch-protection      |
|         |                        | maximal on development and all |                                                                                                                       |
|         |                        | release branches               |                                                                                                                       |
|---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------|
| 10 / 10 | CI-Tests               | 8 out of 8 merged PRs          | https://github.com/ossf/scorecard/blob/70c8e05d6dca69defac0fee1edbbf3fa8732af00/docs/checks.md#ci-tests               |
|         |                        | checked by a CI test -- score  |                                                                                                                       |
|         |                        | normalized to 10               |                                                                                                                       |
|---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------|
| 0 / 10  | CII-Best-Practices     | no effort to earn an OpenSSF   | https://github.com/ossf/scorecard/blob/70c8e05d6dca69defac0fee1edbbf3fa8732af00/docs/checks.md#cii-best-practices     |
|         |                        | best practices badge detected  |                                                                                                                       |
|---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------|
| 4 / 10  | Code-Review            | found 7 unreviewed changesets  | https://github.com/ossf/scorecard/blob/70c8e05d6dca69defac0fee1edbbf3fa8732af00/docs/checks.md#code-review            |
|         |                        | out of 12 -- score normalized  |                                                                                                                       |
|         |                        | to 4                           |                                                                                                                       |
|---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------|
| 10 / 10 | Contributors           | project has 10 contributing    | https://github.com/ossf/scorecard/blob/70c8e05d6dca69defac0fee1edbbf3fa8732af00/docs/checks.md#contributors           |
|         |                        | companies or organizations     |                                                                                                                       |
|---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------|
| 10 / 10 | Dangerous-Workflow     | no dangerous workflow patterns | https://github.com/ossf/scorecard/blob/70c8e05d6dca69defac0fee1edbbf3fa8732af00/docs/checks.md#dangerous-workflow     |
|         |                        | detected                       |                                                                                                                       |
|---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------|
| 0 / 10  | Dependency-Update-Tool | no update tool detected        | https://github.com/ossf/scorecard/blob/70c8e05d6dca69defac0fee1edbbf3fa8732af00/docs/checks.md#dependency-update-tool |
|---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------|
| 10 / 10 | Fuzzing                | project is fuzzed              | https://github.com/ossf/scorecard/blob/70c8e05d6dca69defac0fee1edbbf3fa8732af00/docs/checks.md#fuzzing                |
|---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------|
| 10 / 10 | License                | license file detected          | https://github.com/ossf/scorecard/blob/70c8e05d6dca69defac0fee1edbbf3fa8732af00/docs/checks.md#license                |
|---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------|
| 10 / 10 | Maintained             | 30 commit(s) out of 30 and 4   | https://github.com/ossf/scorecard/blob/70c8e05d6dca69defac0fee1edbbf3fa8732af00/docs/checks.md#maintained             |
|         |                        | issue activity out of 30 found |                                                                                                                       |
|         |                        | in the last 90 days -- score   |                                                                                                                       |
|         |                        | normalized to 10               |                                                                                                                       |
|---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------|
| ?       | Packaging              | packaging workflow not         | https://github.com/ossf/scorecard/blob/70c8e05d6dca69defac0fee1edbbf3fa8732af00/docs/checks.md#packaging              |
|         |                        | detected                       |                                                                                                                       |
|---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------|
| 0 / 10  | Pinned-Dependencies    | dependency not pinned by hash  | https://github.com/ossf/scorecard/blob/70c8e05d6dca69defac0fee1edbbf3fa8732af00/docs/checks.md#pinned-dependencies    |
|         |                        | detected -- score normalized   |                                                                                                                       |
|         |                        | to 0                           |                                                                                                                       |
|---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------|
| 0 / 10  | SAST                   | SAST tool is not run on all    | https://github.com/ossf/scorecard/blob/70c8e05d6dca69defac0fee1edbbf3fa8732af00/docs/checks.md#sast                   |
|         |                        | commits -- score normalized to |                                                                                                                       |
|         |                        | 0                              |                                                                                                                       |
|---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------|
| 10 / 10 | Security-Policy        | security policy file detected  | https://github.com/ossf/scorecard/blob/70c8e05d6dca69defac0fee1edbbf3fa8732af00/docs/checks.md#security-policy        |
|---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------|
| 0 / 10  | Signed-Releases        | 0 out of 5 artifacts are       | https://github.com/ossf/scorecard/blob/70c8e05d6dca69defac0fee1edbbf3fa8732af00/docs/checks.md#signed-releases        |
|         |                        | signed or have provenance      |                                                                                                                       |
|---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------|
| 0 / 10  | Token-Permissions      | detected GitHub workflow       | https://github.com/ossf/scorecard/blob/70c8e05d6dca69defac0fee1edbbf3fa8732af00/docs/checks.md#token-permissions      |
|         |                        | tokens with excessive          |                                                                                                                       |
|         |                        | permissions                    |                                                                                                                       |
|---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------|
| 10 / 10 | Vulnerabilities        | 0 existing vulnerabilities     | https://github.com/ossf/scorecard/blob/70c8e05d6dca69defac0fee1edbbf3fa8732af00/docs/checks.md#vulnerabilities        |
|         |                        | detected                       |                                                                                                                       |
|---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------|

@repokitteh-read-only repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Nov 20, 2023
@repokitteh-read-only
Copy link
Copy Markdown

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).
envoyproxy/dependency-shepherds assignee is @phlax

🐱

Caused by: #30973 was opened by suniltheta.

see: more, trace.

@suniltheta
Copy link
Copy Markdown
Contributor Author

Additional criteria that were not reported by OSSF scorecard

Criteria Answer
Cloud Native Computing Foundation (CNCF) approved license MIT
Dependencies must not substantially increase the binary size unless they are optional (i.e. confined to specific extensions) The dependency is only used in AWS extension and compiled libpugixml.a file is 433K in size.
No duplication of existing dependencies I failed to find any existing xml parser in use within Envoy.
Hosted on a git repository and the archive fetch must directly reference this repository. We will NOT support intermediate artifacts built by-hand located on GCS, S3, etc. https://github.com/zeux/pugixml
CVE history appears reasonable, no pathological CVE arcs No CVE related to pugixml have been registered
Code review (ideally PRs) before merge PRs are reviewed before merge
> 1 contributor responsible for a non-trivial number of commits 62 Contributors
Tests run in CI Tests are run with AppVeyor
High test coverage (also static/dynamic analysis, fuzzing) Fuzzing done
Do other significant projects have shared fate by using this dependency? Unsure
Releases (with release notes) Yes in https://github.com/zeux/pugixml/releases
Commits/releases in last 90 days Yes. Last release was in Oct 1st, 2023

Signed-off-by: Sunil Narasimhamurthy <sunnrs@amazon.com>
Signed-off-by: Sunil Narasimhamurthy <sunnrs@amazon.com>
Copy link
Copy Markdown
Member

@phlax phlax left a comment

Choose a reason for hiding this comment

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

@suniltheta no objections to adding this but would be good to hear from @htuch and @moderation

Comment thread bazel/foreign_cc/BUILD
envoy_cmake(
name = "pugixml",
cache_entries = {
"CMAKE_BUILD_TYPE": "Release",
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

im wondering if this builds anything unwanted - often these cmake builds build everything by default, not just the lib/etc that we need

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks like a better option than the prior option - thanks for researching this

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Let me look into more about how we can avoid unwanted stuff getting included.

@htuch
Copy link
Copy Markdown
Member

htuch commented Nov 21, 2023

No major objection, the dependency seems to meet the rubric from the policy. @suniltheta can I verify that there is no other way to solve this problem? I.e. we can't get JSON equivalent and deal with that on the Envoy side? If that was an option, it would be strictly better in terms of deps.

@suniltheta
Copy link
Copy Markdown
Contributor Author

The solution was based on XML response from AWS documentation https://docs.aws.amazon.com/STS/latest/APIReference/API_AssumeRoleWithWebIdentity.html#API_AssumeRoleWithWebIdentity_Examples

Let me try to dig more if json response is possible. I see this aws cli call getting json response https://docs.aws.amazon.com/cli/latest/reference/sts/assume-role-with-web-identity.html (not sure if xml to json is done by aws cli itself), need to confirm if same is possible by including additional headers.

@suniltheta
Copy link
Copy Markdown
Contributor Author

ok, so simply specifying --header "Accept: application/json" on a call to aws sts, we would get the json response back.

I will update my STS WebIdentity code to adopt to parse the json instead of xml. Thanks @htuch , @moderation, @phlax for the review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

deps Approval required for changes to Envoy's external dependencies

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants