Update usage of unsafe API, upgrade libarchive to 3.7.4, upgrade async http client #120
Conversation
Sources/SwiftlyCore/HTTPClient.swift
Outdated
| try fileHandle.write(contentsOf: bufferPtr) | ||
| let byteData = buffer.getData(at: buffer.readerIndex, length: buffer.readableBytes) | ||
| if let data = byteData { | ||
| try fileHandle.write(contentsOf: data) |
There was a problem hiding this comment.
Can probably replace all of this with
try fileHandle.write(contentsOf: buffer.readableBytesView)There was a problem hiding this comment.
Thank you for the suggestion, this is much cleaner. I've pushed an update that uses it.
cmcgee1024
force-pushed
the
security-fixes
branch
from
2212cbe
to
eb300f2
Compare
Sources/SwiftlyCore/HTTPClient.swift
Outdated
| @@ -185,5 +183,5 @@ public struct SwiftlyHTTPClient { | |||
| } | |||
|
|
|||
| private class HTTPClientWrapper { | |||
| fileprivate let inner = HTTPClient.shared | |||
| fileprivate let inner = HTTPClient(eventLoopGroupProvider: .singleton) | |||
There was a problem hiding this comment.
Note that this was added because in ad hoc testing I discovered an emergent http decompression limit error. It appears that something has changed to trigger it in the GitHub API's.
There was a problem hiding this comment.
Does .shared have different limits to the HTTPClient with a default configuration?
There was a problem hiding this comment.
It appears so. I dug a little deeper because I was concerned that this could be affecting everyone using swiftly. This is the commit that regressed and the testing didn't pick it up at the time: 2d05414
I'm fully reverting that change so that this is not broken, but it does leave some open questions about our commit verification process. Ideally, some form integration testing might have picked this up at the time. Also, it does seem suspect that the shared HTTP Client has a broken decompression limit, whereas this initializer that just reuses the singleton event loop group provider has an intact decompression.
There was a problem hiding this comment.
Have you reported this in the async-http-client repository?
There was a problem hiding this comment.
Including @weissi as I believe this was from his PR to use shared
There was a problem hiding this comment.
@weissi I understand that but we cannot use HTTPClient.shared at the moment. It just keeps throwing errors about compression limits.
There was a problem hiding this comment.
@adam-fowler I've updated this PR with your suggested change to the construction of the http client.
There was a problem hiding this comment.
@adam-fowler Correct right this instant but the fix for this is merged, I'm working with the maintainers on cutting a release
There was a problem hiding this comment.
Thanks to @Lukasa, we've got https://github.com/swift-server/async-http-client/releases/tag/1.21.2 now. So from: "1.21.2" for AHC should fix this.
There was a problem hiding this comment.
@weissi / @Lukasa thanks for pushing that through!
After thinking on this a bit, I don't think that GitHub is the variable as I had initially thought. Everything was working for me until I rebased this change on top of the above commit that changed the way that HTTPClient was initialized. A CI with at least a smoke test that did a swiftly install latest would have picked up the problem.
Prior to the move to swiftlang, we did run tests in CI, a few of which covered that case (testInstallLatest and default-install.sh in particular). That's what leads me to believe maybe something changed on the GitHub side. @shahmishal is working on getting CI set up under the new organization, so we'll have automated testing back shortly.
cmcgee1024
force-pushed
the
security-fixes
branch
from
eb300f2
to
f84dd35
Compare
|
I just looked into the background for the libarchive security issue, and in some ways it's pretty serious as it's related to the known bad actor that compromised the xz library. See libarchive/libarchive#2101 and libarchive/libarchive#1609 for more context. That being said, it looks like no CVE was issued for it, and the suspicious code changes only affected the bsdtar utility, rather than the library functions as used by swiftly. So in light of that, I don't believe there are security concerns here for us. We should still proceed with the upgrade, but I don't think we should indicate that doing so is a security fix in the commit message or release notes, as that has certain implications for downstream users. Likewise, I agree that moving away from using unsafe APIs in swiftly is an improvement that we should merge, but the case identified here was a safe usage of it, so there was no memory safety violation that we're now fixing. In the commit message / release notes, we should note this accordingly (e.g. "stop using unsafe API" rather than "memory safety fixes"). Again, the only reason I'm harping on this is security issues have to be handled with special care, and so if we did have an identified security problem, we would want to cut a release as soon as possible and notify users in the changelog what the case we're fixing is. Thankfully, that doesn't appear to be the case here though. All that being said, changes look good to me mod @adam-fowler's comment about using a singleton. |
patrickfreed
changed the title
|
I took the liberty of updating the PR title, let me know if it seems alright with you. |
cmcgee1024
force-pushed
the
security-fixes
branch
from
f84dd35
to
c25d9b5
Compare
Reduce the use of unsafe swift constructs in favour of ones that are memory safe. In the HTTP client remove the use of the unsafe mutable pointer in favour of direct Data conversion before writing the downloaded file to disk. Upgrade the version of libarchive to 3.7.4, which includes some fixes. Fix the changes to the SwiftlyHTTPClient that changed it to use the shared async HTTPClient. That broke the interactions with the GitHub REST API's. Update to the new async http client that fixes problems with decompression limits.
cmcgee1024
force-pushed
the
security-fixes
branch
from
c25d9b5
to
d8a3482
Compare
cmcgee1024
changed the title
cmcgee1024
changed the title
|
@swift-server-bot test install please |
|
@swift-server-bot test this please |
|
@swift-server-bot test install please |
1 similar comment
|
@swift-server-bot test install please |
|
@swift-server-bot test this please |
|
Thanks everyone for your help with this. |
Reduce the use of unsafe swift constructs in favour of ones that are memory safe. In the HTTP client remove the use of the unsafe mutable pointer in favour of direct Data conversion before writing the downloaded file to disk.
Upgrade the version of libarchive to 3.7.4 with various fixes.
Upgrade the async http client to fix an issue with decompression limits encountered with the GitHub REST API's.