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.

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

Remote access switch #3485

Merged
merged 7 commits into from
Oct 16, 2023
Merged

Conversation

shjala
Copy link
Member

@shjala shjala commented Oct 10, 2023

if we are happy with the overall approach, I will extend the remote access on/off switch to other access points and add reporting to the cloud controller. I can also add a build time option if needed.

@shjala shjala requested a review from rouming as a code owner October 10, 2023 11:14
@shjala shjala requested a review from eriknordmark October 10, 2023 11:15
@codecov
Copy link

codecov bot commented Oct 10, 2023

Codecov Report

Attention: 15 lines in your changes are missing coverage. Please review.

Comparison is base (8fb76fa) 20.53% compared to head (dd45b22) 20.53%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3485      +/-   ##
==========================================
- Coverage   20.53%   20.53%   -0.01%     
==========================================
  Files         202      203       +1     
  Lines       45608    45622      +14     
==========================================
+ Hits         9367     9368       +1     
- Misses      35538    35552      +14     
+ Partials      703      702       -1     
Files Coverage Δ
pkg/pillar/cmd/zedagent/reportinfo.go 0.00% <0.00%> (ø)
pkg/pillar/cmd/zedagent/parseedgeview.go 0.00% <0.00%> (ø)
pkg/pillar/utils/access.go 0.00% <0.00%> (ø)

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@eriknordmark eriknordmark left a comment

Choose a reason for hiding this comment

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

Do we document the various files in /config in some markdown file?

If not, should we document this under edgeview? And/or in SECURITY.md?

@shjala shjala force-pushed the remote_access_switch branch from 43e8086 to 0d99c52 Compare October 10, 2023 14:33
@shjala
Copy link
Member Author

shjala commented Oct 10, 2023

Do we document the various files in /config in some markdown file?

there is CONFIG.md, but I will add a hint in SECURITY.md too.

@shjala shjala requested a review from eriknordmark October 10, 2023 14:39
Makefile Outdated Show resolved Hide resolved
@shjala shjala force-pushed the remote_access_switch branch 2 times, most recently from 09bd75c to b5735e5 Compare October 11, 2023 11:18
@shjala shjala requested a review from milan-zededa as a code owner October 11, 2023 11:18
@shjala shjala force-pushed the remote_access_switch branch 2 times, most recently from 892e655 to 9f7ffe9 Compare October 11, 2023 12:11
@shjala shjala force-pushed the remote_access_switch branch from 9f7ffe9 to 2acf32c Compare October 11, 2023 13:24
@shjala shjala requested a review from rouming October 11, 2023 13:25
@shjala shjala force-pushed the remote_access_switch branch 2 times, most recently from 981e107 to bc2a1f3 Compare October 11, 2023 13:31
Copy link
Contributor

@rouming rouming left a comment

Choose a reason for hiding this comment

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

Ack. Please don't forget to split the doc part and the code part on two different commits. Thanks.

@shjala shjala changed the title [WIP] Remote access switch Remote access switch Oct 11, 2023
@shjala shjala force-pushed the remote_access_switch branch 2 times, most recently from 27d29ed to 59ba375 Compare October 11, 2023 14:27
@shjala
Copy link
Member Author

shjala commented Oct 11, 2023

Waiting for this lf-edge/eve-api#31 to merge to add the "report status to cloud" part.

@shjala shjala force-pushed the remote_access_switch branch 2 times, most recently from 9cc5386 to ec249bc Compare October 12, 2023 07:24
Copy link
Contributor

@rouming rouming left a comment

Choose a reason for hiding this comment

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

Ack. Thanks for commit split, looks nicer and easier to read.

Copy link
Contributor

@eriknordmark eriknordmark left a comment

Choose a reason for hiding this comment

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

LGTM. Some doc nits.

docs/SECURITY.md Outdated Show resolved Hide resolved
@shjala shjala force-pushed the remote_access_switch branch from ec249bc to cf33bed Compare October 12, 2023 09:40
Copy link
Contributor

@eriknordmark eriknordmark left a comment

Choose a reason for hiding this comment

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

Run eden again

@@ -13,6 +13,7 @@ In general, EVE is trying to make sure that its controller always has the last w
* `wpa_supplicant.conf` - a legacy way of configuring EVE's WiFi
* `authorized_keys` - initial authorized SSH keys for accessing EVE's debug console; DO NOT use options, we only accept 'keytype, base64-encoded key, comment' format
* `bootstrap-config.pb`- initial device configuration used only until device is onboarded (see below for details)
* `remote_access_disabled`- a file indicating remote access status, if it exist remote access (edge-view and ssh) is disabled. Please check [config document](SECURITY.md#disabling-remote-access) for more information.
Copy link
Contributor

Choose a reason for hiding this comment

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

@shjala , is there any particular reason to use a file for this feature (disabled remote access)? I'm wondering if it couldn't be integrated to the config properties? Also, I'm wondering if you could use this file to hold more config options (for remote access), instead of just mark it as disabled.... for instance, the file could be named as "remote_access", and contains inside the enable/disable option along with other properties (if that's the case).... I'm not against the current implementation though...

Copy link
Member Author

Choose a reason for hiding this comment

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

I save that for a future PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

@rene The requirement is that it can not be possible to change this from the controller; only a user with local access should be able to enable it. The config properties are all about changes from the controller.

We could make the file more generic, but that means more care (writeRename) to avoid ending up with a corrupted file when there is a power outage. And we don't know what the scope would be for future items which would have the same requirement for local-only modifications. There might be none.

if [ -f "/config/remote_access_disabled" ]; then
# this is picked up by newlogd
echo "Remote access disabled, ssh server not started" > /dev/kmsg
while true; do
Copy link
Contributor

Choose a reason for hiding this comment

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

you can just use sleep infinity here, which will block the process indefinitely....

Copy link
Member Author

Choose a reason for hiding this comment

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

are you sure busybox implements all the GNU version options?

Copy link
Contributor

Choose a reason for hiding this comment

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

I checked and it works:
006318ad-dfdc-4a49-87d6-9066cc832b48:# which sleep
/bin/sleep
006318ad-dfdc-4a49-87d6-9066cc832b48:
# ls -l /bin/sleep
lrwxrwxrwx 1 root root 12 Sep 12 2022 /bin/sleep -> /bin/busybox
006318ad-dfdc-4a49-87d6-9066cc832b48:~# sleep infinity

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks, changed it.

@eriknordmark
Copy link
Contributor

I guess @rene 's sleep comment is what remains. Passsed the eden test suite (apart from the tests which always fails).

@shjala
Copy link
Member Author

shjala commented Oct 16, 2023

can't see why yetus failed, there is this in the logs :
ERROR: Failed to write github status. Token expired or missing repo:status write?
I'll run it locally.

@shjala
Copy link
Member Author

shjala commented Oct 16, 2023

can't see why yetus failed, there is this in the logs : ERROR: Failed to write github status. Token expired or missing repo:status write? I'll run it locally.

ran it locally and checked it manually, can't see any complaint about lines I have changed.

@eriknordmark
Copy link
Contributor

can't see why yetus failed, there is this in the logs : ERROR: Failed to write github status. Token expired or missing repo:status write? I'll run it locally.

yetus log shows
ERROR: GH:3485 does not apply to master.

So need to rebase on master.

Signed-off-by: Shahriyar Jalayeri <shahriyar@zededa.com>
Signed-off-by: Shahriyar Jalayeri <shahriyar@zededa.com>
Signed-off-by: Shahriyar Jalayeri <shahriyar@zededa.com>
Signed-off-by: Shahriyar Jalayeri <shahriyar@zededa.com>
Signed-off-by: Shahriyar Jalayeri <shahriyar@zededa.com>
Signed-off-by: Shahriyar Jalayeri <shahriyar@zededa.com>
sleep to infinity (INT_MAX) instead of just a day.

Signed-off-by: Shahriyar Jalayeri <shahriyar@zededa.com>
@shjala shjala force-pushed the remote_access_switch branch from 3e36e00 to dd45b22 Compare October 16, 2023 13:24
@shjala shjala requested a review from eriknordmark October 16, 2023 13:25
@eriknordmark eriknordmark merged commit 571490b into lf-edge:master Oct 16, 2023
14 of 16 checks passed
@shjala shjala deleted the remote_access_switch branch October 17, 2023 07:56
shjala added a commit to shjala/eve that referenced this pull request Oct 18, 2023
This is backport of lf-edge#3485,
which adds remote access switch to eve, blocking ssh and
edge-view if remote access is disabled.

Signed-off-by: Shahriyar Jalayeri <shahriyar@zededa.com>
shjala added a commit to shjala/eve that referenced this pull request Oct 19, 2023
This is backport of lf-edge#3485,
which adds remote access switch to eve, blocking ssh and edge-view if
remote access is disabled.

Signed-off-by: Shahriyar Jalayeri <shahriyar@zededa.com>
// RemoteAccessDisabled checks if remote access is enabled/disabled
// by checking if the file /config/remote_access_disabled exists or not.
func RemoteAccessDisabled() bool {
if _, err := os.Stat(types.RemoteAccessFlagFileName); err == nil {
Copy link
Contributor

@christoph-zededa christoph-zededa Oct 19, 2023

Choose a reason for hiding this comment

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

Suggested change
if _, err := os.Stat(types.RemoteAccessFlagFileName); err == nil {
if FileExists(nil, types.RemoteAccessFlagFileName) {

from utils package

eriknordmark pushed a commit that referenced this pull request Oct 19, 2023
This is backport of #3485,
which adds remote access switch to eve, blocking ssh and edge-view if
remote access is disabled.

Signed-off-by: Shahriyar Jalayeri <shahriyar@zededa.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants