-
Notifications
You must be signed in to change notification settings - Fork 79
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.
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
allow building ssh3-server on other unix platform #56
Conversation
.github/workflows/build.yml
Outdated
@@ -78,7 +78,34 @@ jobs: | |||
run: go get ./... | |||
# No Build Server on MacOS right now as the server currently does no build on MacOS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this comment can go now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch, thank you
a520dc6
to
f492ea5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good overall. Is there a way to test the workflows before merging?
.github/workflows/build.yml
Outdated
- name: Install dependencies | ||
run: go get ./... | ||
# No Build Server on MacOS right now as the server currently does no build on MacOS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# No Build Server on MacOS right now as the server currently does no build on MacOS |
56fceda
to
c4db6bf
Compare
util/unix_util/linux_user.go
Outdated
|
||
It also checks whether those files have insecure UNIX permissions. | ||
*/ | ||
//go:build linux && (amd64 || arm64) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍🏻
@@ -0,0 +1,47 @@ | |||
//go:build unix && (!linux || (!amd64 && !arm64)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not following what this is saying. Are you trying to build on !linux? What's the architecture checks adding here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That should be removed at some point, I added these checks to make the release script that does cross-compilation (.github/workflows/build-release.yml
) work correctly as currently this script cannot cross-compile for anything else than linux arm64.
These checks are present so that we use non_password_auth_user.go
for macos, bsd and linux other than amd64 and arm64. This file allows compiling the server without the CGO code used for password-based auth. That basically provides a server with password-based auth disabled.
For linux amd64 and arm64, util/unix_util/linux_user.go
is used instead. I agree that every linux should compile correctly using util/unix_util/linux_user.go
when compiled natively, but that would require a correct cross-compilation toolchain in .github/workflows/build-release.yml
to make it work when building releases for non-amd64/arm64 archs.
So I know, right now the code is a bit tied to the release-building script until we manage to get rid of CGO. But maybe you know a cleaner way do make that work ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting question there. I think the issue you're trying to solve is how to build on platforms without the necessary headers or os features available in github action CI. I think the build flag approach is a good idea here. You could rely on platform specific installers to solve the edge cases like apt, homebrew.
I didn't see what functionality was offered with cgo linux
. Is it just password auth (pam)? That's not a super interesting feature for ssh
, so we could offer binaries without it. If you want pam
then build from source go install ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is just password auth, yes. Right now, it's using /etc/shadow
through glibc's getpwnam
, and not PAM.
PAM would still be cool to have at it provides some features such as motd or even 2-factor auth. There are packages such as this one that looks promising but it relies on libpam.so
.
That's not a super interesting feature for ssh, so we could offer binaries without it. If you want pam then build from source go install ...
Yes, right now we have this pre-release that ships a server without password auth for macos, openbsd, freebsd and linux i386 and armv6.
I still think would be cool to have PAM at some point though but that would mean we can't get rid of CGO (at least in our dependencies).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added the disable_password_auth
build tag that can be used on Linux when cross-compiling without full CGO support. (cf 5cdd68b)
af66c8e
to
c462183
Compare
This is a small rework on server-side authentication mechanisms to allow compiling the server on non-linux Unix platforms. It simply remove any CGO dependency, and, to do so, disables password-based autuentication on non-linux servers.
000f185
to
2365964
Compare
This is a small rework on server-side authentication mechanisms to allow compiling the server on non-linux Unix platforms. It simply remove any CGO dependency, and, to do so, disables password-based autuentication on non-linux servers. Relates to and should partially fix #18.