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

Add support for JRE provisioning: Jre tar.gz unpack #2036

Merged
merged 12 commits into from
Jul 12, 2024
Merged

Add support for JRE provisioning: Jre tar.gz unpack #2036

merged 12 commits into from
Jul 12, 2024

Conversation

gregory-paidis-sonarsource
Copy link
Contributor

Fixes #2031

@gregory-paidis-sonarsource gregory-paidis-sonarsource force-pushed the greg/JRE_Download_TarGzUnpack branch from 8eeed83 to 0aee8b5 Compare July 9, 2024 13:46
@gregory-paidis-sonarsource gregory-paidis-sonarsource linked an issue Jul 9, 2024 that may be closed by this pull request
Add support for JRE provisioning: Jre tar.gz unpack #2031
Closed
@martin-strecker-sonarsource martin-strecker-sonarsource force-pushed the Martin/JRE_Download_ZipUnpack branch from 11cf39a to 173a9d7 Compare July 9, 2024 13:52
@gregory-paidis-sonarsource gregory-paidis-sonarsource force-pushed the greg/JRE_Download_TarGzUnpack branch 3 times, most recently from 03d3f67 to 19e40b6 Compare July 9, 2024 15:03
gregory-paidis-sonarsource
gregory-paidis-sonarsource commented Jul 9, 2024
View reviewed changes
src/SonarScanner.MSBuild.PreProcessor/JreCaching/TarGzUnpacker.cs Outdated
tar.CopyEntryContents(outputStream);

#if NETSTANDARD
if (operatingSystemProvider.OperatingSystem() is PlatformOS.Linux or PlatformOS.Alpine)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this section cannot be covered by tests, since we run tests only targeting .NET Framework 4.6.2.

@gregory-paidis-sonarsource gregory-paidis-sonarsource force-pushed the greg/JRE_Download_TarGzUnpack branch from 19e40b6 to 282751a Compare July 9, 2024 15:16
@martin-strecker-sonarsource martin-strecker-sonarsource force-pushed the Martin/JRE_Download_ZipUnpack branch from ffd1271 to f3a994b Compare July 9, 2024 18:40
Base automatically changed from Martin/JRE_Download_ZipUnpack to master July 10, 2024 07:24
@gregory-paidis-sonarsource gregory-paidis-sonarsource force-pushed the greg/JRE_Download_TarGzUnpack branch 3 times, most recently from 9efd04b to 7138c77 Compare July 10, 2024 09:02
@gregory-paidis-sonarsource gregory-paidis-sonarsource force-pushed the greg/JRE_Download_TarGzUnpack branch from 7138c77 to 7dc4ab4 Compare July 10, 2024 09:21
costin-zaharia-sonarsource
costin-zaharia-sonarsource reviewed Jul 10, 2024
View reviewed changes
Tests/SonarScanner.MSBuild.PreProcessor.Test/JreCaching/TarGzUnpackTests.cs Outdated Show resolved Hide resolved
src/SonarScanner.MSBuild.PreProcessor/JreCaching/UnpackerFactory.cs Outdated
};

private static bool IsTarball(string filename) =>
string.Equals(Path.GetExtension(Path.GetFileNameWithoutExtension(filename)), ".tar", StringComparison.OrdinalIgnoreCase);
Copy link
Member

Choose a reason for hiding this comment

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

I do not like the fact that we have to call ToUpperInvariant and also that we do an indirect string match. Part in the Create method and part in IsTarball.

What do you think about rewriting the Create method body to:

        archive switch
        {
            _ when archive.EndsWith(".ZIP", StringComparison.OrdinalIgnoreCase) => new ZipUnpacker(),
            _ when archive.EndsWith(".TAR.GZ", StringComparison.OrdinalIgnoreCase) => new TarGzUnpacker(directoryWrapper, fileWrapper, operatingSystemProvider),
            _ => null
        };

I would also rename archive to archivePath or something similar to better explain that it's the file name and not the content of the archive.

src/SonarScanner.MSBuild.PreProcessor/JreCaching/TarGzUnpacker.cs Outdated
var destinationFullPath = Path.GetFullPath(destinationDirectory).TrimEnd('/', '\\');
while (tarIn.GetNextEntry() is {} entry)
{
if (entry.TarHeader.TypeFlag is not TarHeader.LF_LINK or TarHeader.LF_SYMLINK)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (entry.TarHeader.TypeFlag is not TarHeader.LF_LINK or TarHeader.LF_SYMLINK)
if (entry.TarHeader.TypeFlag is not TarHeader.LF_LINK and not TarHeader.LF_SYMLINK)

or

Suggested change
if (entry.TarHeader.TypeFlag is not TarHeader.LF_LINK or TarHeader.LF_SYMLINK)
if (entry.TarHeader.TypeFlag is not (TarHeader.LF_LINK or TarHeader.LF_SYMLINK))

costin-zaharia-sonarsource
costin-zaharia-sonarsource reviewed Jul 10, 2024
View reviewed changes
src/SonarScanner.MSBuild.PreProcessor/JreCaching/TarGzUnpacker.cs Outdated
{
_ = new Mono.Unix.UnixFileInfo(destinationFile)
{
FileAccessPermissions = (Mono.Unix.FileAccessPermissions)entry.TarHeader.Mode // set the same permissions as inside the archive
Copy link
Member

Choose a reason for hiding this comment

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

@martin-strecker-sonarsource, the reason why we don't use TarArchive.CreateInputTarArchive(tarIn).EtractContents(); is to be able to copy the file permission settimgs? (just trying to understand the use case)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. The alternative is to use EtractContents (which has path traversal protection) and do a second pass, where we do the permission setting (and we know in the second pass that there is no path traversal in the archive).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it is fine to do it like this, as it is not that big and it is quite readable.

gregory-paidis-sonarsource and others added 7 commits July 11, 2024 12:15
@gregory-paidis-sonarsource
Add support for JRE provisioning: Delete archive when checksum valida…
4dc24f5 
…tion fails. (#2033)

Add logging and log assertions.

Implement and test zip unpack.

Add assertion

Remove verbatim string token

Normalize line endings

Fix doc comment

NewLine

Fix doc comment

Revert change in UT after re-base

Add unpackProvider dependency to JreCache

Use TryGetUnpack early to bail out.

Unpack provider integration tests

Add comments

Add zipslip test

Assert good file as well

Add ExtractToDirectory

Re-work to use "ExtractToDirectory" which handles all kind of cases much better than we can do.

Implement UnpackJre in JreCache

Fix flow and assertions

Add success test

Add more assertions

Add TODOs

Add temp folder clean up and tests

Add todos

Add Unpack_Failure_JavaExeNotFound

Fx todo

Cleanup

Add EndToEndTestWithFiles_Success test

Fix path in JreDescriptor

Newlines

Apply suggestions from code review

Move NormalizeLineEndings to test project

Reename IUnpackProvider to IUnpackerFactory

Rename IUnpack to IUnpacker

Rename factory method

Rename GetUnpackForArchive to CreateForArchive

Use resx for all messages

Fix tests after rename

Return null instead of throw for CreateForArchive

Simplify CreateForArchive call handling

Exract path caclualtions

Remove string.Format

Re-arrange log assertions in UnpackerFactory_Success and add comment

Remove file init

Add result assertion to EndToEndTestWithFiles_Success

Inline substitutes in UnpackerFactoryTests

Use Directory.Delete(.., true) for cleanup.

Simplify file/directoy assertions in EndToEndTestWithFiles_Success

Fixes after re-base

Remove outdated comment
@gregory-paidis-sonarsource
Add support for JRE provisioning: Jre tar.gz unpack
f9c3dbd
@gregory-paidis-sonarsource
Review 1
e183dd3
@gregory-paidis-sonarsource
Add a test for rooted apths
72feb76
@gregory-paidis-sonarsource
Fix boolean condition wrong logic
75484e2
@gregory-paidis-sonarsource
Remove empty line
3576608
@gregory-paidis-sonarsource
Review 2
49e4434
@gregory-paidis-sonarsource gregory-paidis-sonarsource force-pushed the greg/JRE_Download_TarGzUnpack branch from 20f7e0b to 49e4434 Compare July 11, 2024 10:17
@gregory-paidis-sonarsource
Copy link
Contributor Author

@costin-zaharia-sonarsource just FYI:
@martin-strecker-sonarsource suggested to extract the file permissions logic to a server, so that we can inject it and test that it is actually called.
I agree with the suggestion, but to save some time on reviewing (plus Martin is off tomorrow), I left a couple TODOs and I plan to tackle them on the next PR (that you will review, tomorrow).
If you agree with this, let me know and I will create the card for it.

@gregory-paidis-sonarsource gregory-paidis-sonarsource force-pushed the greg/JRE_Download_TarGzUnpack branch from 207e214 to 6f8c584 Compare July 11, 2024 15:03
@gregory-paidis-sonarsource gregory-paidis-sonarsource mentioned this pull request Jul 11, 2024
Closed
martin-strecker-sonarsource
martin-strecker-sonarsource approved these changes Jul 11, 2024
View reviewed changes
Copy link
Contributor

@martin-strecker-sonarsource martin-strecker-sonarsource left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM, nut a few more tests are needed.

Tests/SonarScanner.MSBuild.PreProcessor.Test/JreCaching/JreCacheTests.cs Show resolved Hide resolved
Tests/SonarScanner.MSBuild.PreProcessor.Test/JreCaching/TarGzUnpackTests.cs Show resolved Hide resolved
src/SonarScanner.MSBuild.Common/Interfaces/IOperatingSystemProvider.cs Show resolved Hide resolved
@gregory-paidis-sonarsource gregory-paidis-sonarsource enabled auto-merge (squash) July 12, 2024 07:54
@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
5 Accepted issues

Measures
0 Security Hotspots
94.1% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@gregory-paidis-sonarsource gregory-paidis-sonarsource merged commit 3c6df25 into master Jul 12, 2024
13 checks passed
@gregory-paidis-sonarsource gregory-paidis-sonarsource deleted the greg/JRE_Download_TarGzUnpack branch July 12, 2024 08:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@costin-zaharia-sonarsource costin-zaharia-sonarsource costin-zaharia-sonarsource left review comments

@martin-strecker-sonarsource martin-strecker-sonarsource martin-strecker-sonarsource 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.

Add support for JRE provisioning: Jre tar.gz unpack
3 participants
@gregory-paidis-sonarsource @costin-zaharia-sonarsource @martin-strecker-sonarsource