feat: Add support of configuring host to use Satellite server #170

jirihnidek · 2024-11-21T14:25:15Z

Card ID: CCT-993
Introduced new command "configure" and immediately added sub-command "satellite" to this command. "satellite" subcommand has --url CLI option and you can set hostname or URL of server Satellite server.
- When valid URL or hostname is provided, then rhc tries to read /api/ping endpoint and it tries to verify that given server is Satellite server
- When Satellite server is verified, then it tries to download bootstrap script from /pub/katello-rhsm-consumer
- When script is downloaded, then this script is run as root user.
Is little bit risky to run some script downloaded from web server. For this reason we will try to use different approach in the future. Satellite server will provide public REST API endpoints providing CA certs and rendered rhsm.conf. We will simply download these files and install them to system. Unfortunately, these REST API endpoints will be available in some future version of Satellite.

jirihnidek · 2024-11-21T14:35:12Z

I'm not sure about following things:

Should we be more interactive, when --url is not provided? We print error, when no --url is provided.
The bash script is downloaded to /var/lib/rhc and it is deleted after script is run. Should we keep the file? Or should we keep the file, when some hidden CLI option is used (e.g. --keep-bootstrap-file)?
Should we return errors in machine readable format too?

TODO:

I would like to call both REST API call in parallel, but we can do it in another PR.

ezr-ondrej · 2024-11-25T11:15:02Z

/retest

subpop

I have not read over satellite.go yet. I'll get to that in a follow up review. I left a scattering of comments through the other files. I have not tested any of the functionality yet. This is just from a reading over of the code.

main.go

configure_cmd.go

subpop · 2024-11-27T16:04:16Z

configure_cmd.go

+		s.Suffix = fmt.Sprintf(" Configuring '%v' to use Satellite server: %v", hostname, satelliteUrl.Host)
+	}
+	// Run the script. It should install CA certificate, change configuration of rhsm.conf.
+	// In theory, it can do almost anything.


Scary. We should follow this up with a distributed SELinux policy to restrict the scope execution for rhc in general.

subpop · 2024-11-27T20:22:34Z

I would also like to take a UX pass over the strings in the new command. We should ensure consistent terminology as much as we can.

jirihnidek · 2024-11-28T09:59:47Z

I would also like to take a UX pass over the strings in the new command. We should ensure consistent terminology as much as we can.

OK, I updated the PR.

chris1984 · 2024-12-04T20:29:39Z

Hi, Can we be sure to support multiple activation keys when registering to Satellite?

jirihnidek · 2024-12-05T08:24:12Z

Hi, Can we be sure to support multiple activation keys when registering to Satellite?

Hi Chris, rhc connect already supports registration using multiple activation keys.

You can use CLI option --activation-key multiple times:

[root@rhel10 ~]# rhc connect --activation-key KEY-01 --activation-key KEY-02 --organization MY-ORG

It is also emphasize in the help of connect command:

[root@rhel10 ~]# rhc connect --help
NAME:
   rhc connect - Connects the system to Red Hat

USAGE:
   rhc connect [command options]

DESCRIPTION:
   The connect command connects the system to Red Hat Subscription Management, Red Hat Insights and Red Hat and activates the yggdrasil service that enables Red Hat to interact with the system. For details visit: https://red.ht/connector

OPTIONS:
   --username USERNAME, -u USERNAME                               register with USERNAME
   --password PASSWORD, -p PASSWORD                               register with PASSWORD
   --organization ID, -o ID                                       register with ID
   --activation-key KEY, -a KEY [ --activation-key KEY, -a KEY ]  register with KEY
   --format value, -f value                                       prints output of connection in machine-readable format (supported formats: "json")
   --help, -h                                                     show help

subpop

The satellite.go code feels disjointed. I think we could clean up the function pattern by making a dedicated HTTP client and building some methods on that. This way the two requests we use today (ping and /pub/katello-rhsm-consumer) can share the same HTTP client (TLS configuration, base URL, etc).

Then this client can be instantiated from within satelliteAction and used to perform the various operations it wants to.

The ping method feels incomplete too. If the response body is not documented on the Internet somewhere, I don't like unmarshalling all those fields, especially if they are not used. We could probably get away with only a status field, if we're checking for liveness by sending the server a ping.

subpop · 2024-12-09T14:58:10Z

configure_cmd.go

+			configureSatelliteResult.HostnameError = err.Error()
+			return cli.Exit(configureSatelliteResult, exitCode)
+		} else {
+			return cli.Exit(err, exitCode)


Do we need a wrapping error message here?

Suggested change

return cli.Exit(err, exitCode)

return cli.Exit(fmt.Errorf("could not acquire hostname: %w", err), exitCode)

configure_cmd.go

satellite.go

subpop · 2024-12-09T15:05:33Z

satellite.go

+}
+
+// Error implement error interface for structure ConfigureSatelliteResult
+func (configureSatelliteResult ConfigureSatelliteResult) Error() string {


There is a Go convention to refer to the 'self' object in a method as a single
or small variable name. I think this could be made a bit more concise by
replacing configureSatelliteResult with result. Maybe then rename the
existing result string to message or something like that.

subpop · 2024-12-09T15:27:10Z

satellite.go

+	}
+	transport := http.DefaultTransport.(*http.Transport).Clone()
+	transport.TLSClientConfig = &tlsConfig
+	client := &http.Client{Transport: transport}


Is this same client (with its TLS configuration) not required to download the
script in downloadSatelliteScript?

It seems that it is possible to access /pub using (http and https). It is possible to access /api/ping using only https. In theory we can try to use one client using only one https connection.

Yea, I think we can consolidate the HTTP clients into a specialized one for
performing Satellite REST requests.

subpop · 2024-12-09T15:29:44Z

satellite.go

+		return fmt.Errorf("unable to parse ping satellite server response: %v", err)
+	}
+
+	// In theory, we could check for the status of candlepin and candlepin auth, etc.,


Is it safe to assume that if the HTTP server responds with 200, that's an
appropriate response to a "ping" request?

I can delete this commented block of code.

In theory we can also remove json.Unmarshal() of the body, but as I already said, it is safer to do more checks that the server is really Satellite server than do less checks, because we run downloaded script as a root.

If user provide full URL to some no-satellite script and there will be also REST API endpoint /api/ping (not so unlikely), then the script will be downloaded and run as a root user. For this reason I would rather check that returned object of /api/ping is really response from Satellite.

That makes sense to me. Considering the risk we're taking here (downloading a
shell script and running it as root), we should do as much as we can to ensure
that this script is coming from a trusted source.

jirihnidek · 2024-12-10T15:13:47Z

The satellite.go code feels disjointed. I think we could clean up the function pattern by making a dedicated HTTP client and building some methods on that. This way the two requests we use today (ping and /pub/katello-rhsm-consumer) can share the same HTTP client (TLS configuration, base URL, etc).

Do you mean to reuse this https://github.com/RedHatInsights/rhc/blob/main/internal/http/client.go again? It seems that this file is unused ATM. It will probably require to extend this file to be able to use it for communication with Satellite server.

subpop

The satellite.go code feels disjointed. I think we could clean up the function pattern by making a dedicated HTTP client and building some methods on that. This way the two requests we use today (ping and /pub/katello-rhsm-consumer) can share the same HTTP client (TLS configuration, base URL, etc).

Do you mean to reuse this https://github.com/RedHatInsights/rhc/blob/main/internal/http/client.go again? It seems that this file is unused ATM. It will probably require to extend this file to be able to use it for communication with Satellite server.

Maybe not reuse it directly, but create a SatelliteHTTPClient type that preconfigures the TLS configuration used by a specific backing HTTP client, and that has methods attached to it to make the REST API calls.

type SatelliteHTTPClient struct {
    client *http.Client
}

func (s *SatelliteHTTPClient) Ping() (SatelliteHTTPPingResponse, error) { ... }

It might seem like a lot more boilerplate code up front, but I think it will position this command to evolve more Satellite features in the future without needing to refactor things.

subpop · 2025-01-09T15:03:11Z

constants.go

@@ -39,6 +39,7 @@ var (
 	SysconfDir        string
 	LocalstateDir     string
 	DbusInterfacesDir string
+	VarLibDir         string


There's something about this name that doesn't sit right with me. "VarLibDir" makes me think /var/lib, not /var/lib/rhc. I'd rather we name it something more accurate, like RhcLocalstateDir.

subpop

There's a lint error caught by the linting actions I noticed, and I left a few comments here and there. I'll get to some functional testing shortly.

subpop · 2025-01-09T15:09:20Z

satellite.go

+	// We have to use insecure HTTPs connection, because most of the customers use
+	// self-signed certificates
+	tlsConfig := tls.Config{
+		InsecureSkipVerify: true,
+	}


So we're downloading a shell script from a URL and executing it as root, and we're downloading that shell script from an unsigned HTTPS endpoint. 😱

Do customers actually use self-signed certificates? Or do they use certificates that are signed by an internal/private signing authority? Could we expose the option to add a CA root certificate as part of this TLS configuration so that customers can at least use their own signing authority? And maybe make them opt-in to fully disabling the certificate verification?

subpop · 2025-01-09T15:12:35Z

satellite.go

+	return &satellitePingResponse, nil
+}
+
+// downloadSatelliteScript tries to download script from provided URL to given file


This method documentation looks like a leftover from the previous implementation. It doesn't match the current method name and it describes a parameter that it no longer accepts.

Suggested change

// downloadSatelliteScript tries to download script from provided URL to given file

// downloadScript tries to download script from provided URL to given file

subpop · 2025-01-09T15:19:20Z

configure_cmd.go

+	}
+
+	satelliteUrlStr := ctx.String("url")
+	if satelliteUrlStr == "" {


We can mark the cli.Flag defining this flag as Required: true. That makes this check unnecessary.

subpop · 2025-01-09T15:19:45Z

main.go

+						&cli.StringFlag{
+							Name:    "url",
+							Usage:   "URL of the Satellite server",
+							Aliases: []string{"u"},


This flag should be marked as required:

Suggested change

Aliases: []string{"u"},

Aliases: []string{"u"},

Required: true,

subpop · 2025-01-09T15:21:35Z

configure_cmd.go

+		return fmt.Errorf("cannot configure connected system to use satellite; disconnect system firtst")
+	}
+
+	return checkForUnknownArgs(ctx)


I don't know if I've commented on this before, but I like this pattern of returning a call through another function. This function is named very well and this clearly describes what happens through this function. Nicely written.

subpop · 2025-01-09T16:04:47Z

main.go

@@ -174,6 +174,40 @@ func main() {
 			Before:      beforeConnectAction,
 			Action:      connectAction,
 		},
+		{
+			Name:      "configure",
+			Usage:     "Configure the host",


Looking at this text surrounded by the other text in the help output, I think
this needs to change to something like:

"Configures the behavior of the system"

COMMANDS: connect Connects the system to Red Hat configure Configure the host disconnect Disconnects the system from Red Hat status Prints status of the system's connection to Red Hat help, h Shows a list of commands or help for one command

subpop

Some thoughts on the user experience.

I definintely don't like the insecure certificate experience. I first set up my instance like so: rhc configure satellite --url https://my-satelite.local and got the error back:

could not download satellite bootstrap script: could not download file https://my-satellite.local/pub/katello-rhsm-consumer : Get "https://my-satellite.local/pub/katello-rhsm-consumer": tls: failed to verify certificate: x509: certificate signed by unknown authority

We need to expect users will encounter this, and should provide two possible solutions:

The ability to add a certificate authority to the TLS configuration so that the server certificate is valid (--ca-root)
The ability to opt-out of all security (--insecure)

I'm not pleased with the idea of a (2), but I know our options are limited with the scope of host identification we get from Satellite currently. That being said, if we are going to allow for the insecure downloading of content, Id' much rather we do so with explicit opt-in from the user.

My execution of the script failed. All I got on the console was: execution of 0x40001eeab0 script failed: exit status 127 Unless we know this script is robust enough to never fail, we really need to provide a means for users to diagnose these errors. At the very least, let's pipe the stdout and stderr of the script to a log file. This error message isn't descriptive enough for a user to start troubleshooting.

* Card ID: CCT-993 * Introduced new command "configure" and immediately added sub-command "satellite" to this command. "satellite" subcommand has --url CLI option and you can set hostname or URL of server Satellite server. * When valid URL or hostname is provided, then rhc tries to read /api/ping endpoint and it tries to verify that given server is Satellite server * When Satellite server is verified, then it tries to download bootstrap script from /pub/katello-rhsm-consumer to directory /var/lib/rhc * When script is downloaded, then this script is run as root user. The "/usr/bin/bash" is used for running script. * When running of script is finished, then this script is deleted unless hidden CLI option --keep-artifacts is used * It is not possible to run this command, when system is connected, because it would not be possible tounregister or disconnect system, if configuring was allowed on connected system. The behavior of rhc/subscription-manager would be strange * It is little bit risky to run some script downloaded from web server. For this reason we will try to use different approach in the future. Satellite server will provide public REST API endpoints providing CA certs and rendered rhsm.conf. We will simply download these files and install them to system. Unfortunately, these REST API endpoints will be available in some future version of Satellite. * Directory /var/lib/rhc is created during installation of RPM package

jirihnidek requested a review from subpop November 21, 2024 14:38

jirihnidek force-pushed the jhnidek/satellite branch from 48699ce to 409c595 Compare November 22, 2024 15:13

subpop requested changes Nov 27, 2024

View reviewed changes

jirihnidek force-pushed the jhnidek/satellite branch from 409c595 to 2413479 Compare November 28, 2024 09:59

subpop self-requested a review December 2, 2024 14:43

jirihnidek force-pushed the jhnidek/satellite branch from 2413479 to c69b666 Compare December 6, 2024 14:21

subpop requested changes Dec 9, 2024

View reviewed changes

subpop reviewed Dec 12, 2024

View reviewed changes

jirihnidek force-pushed the jhnidek/satellite branch from c69b666 to 9e0dcaf Compare January 9, 2025 14:46

subpop reviewed Jan 9, 2025

View reviewed changes

subpop requested changes Jan 9, 2025

View reviewed changes

subpop reviewed Jan 9, 2025

View reviewed changes

jirihnidek force-pushed the jhnidek/satellite branch from 9e0dcaf to 974ef6d Compare January 10, 2025 21:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add support of configuring host to use Satellite server #170

feat: Add support of configuring host to use Satellite server #170

jirihnidek commented Nov 21, 2024

jirihnidek commented Nov 21, 2024 •

edited

Loading

ezr-ondrej commented Nov 25, 2024

subpop left a comment

subpop Nov 27, 2024

subpop commented Nov 27, 2024

jirihnidek commented Nov 28, 2024

chris1984 commented Dec 4, 2024

jirihnidek commented Dec 5, 2024

subpop left a comment

subpop Dec 9, 2024

subpop Dec 9, 2024

subpop Dec 9, 2024

jirihnidek Dec 10, 2024

subpop Dec 12, 2024

subpop Dec 9, 2024

jirihnidek Dec 10, 2024

jirihnidek Dec 10, 2024

subpop Dec 12, 2024

jirihnidek commented Dec 10, 2024

subpop left a comment

subpop Jan 9, 2025

subpop left a comment

subpop Jan 9, 2025

subpop Jan 9, 2025

subpop Jan 9, 2025

subpop Jan 9, 2025

subpop Jan 9, 2025

subpop Jan 9, 2025

subpop left a comment

	return cli.Exit(err, exitCode)
	return cli.Exit(fmt.Errorf("could not acquire hostname: %w", err), exitCode)

	// downloadSatelliteScript tries to download script from provided URL to given file
	// downloadScript tries to download script from provided URL to given file

	Aliases: []string{"u"},
	Aliases: []string{"u"},
	Required: true,

feat: Add support of configuring host to use Satellite server #170

Are you sure you want to change the base?

feat: Add support of configuring host to use Satellite server #170

Conversation

jirihnidek commented Nov 21, 2024

jirihnidek commented Nov 21, 2024 • edited Loading

ezr-ondrej commented Nov 25, 2024

subpop left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

subpop commented Nov 27, 2024

jirihnidek commented Nov 28, 2024

chris1984 commented Dec 4, 2024

jirihnidek commented Dec 5, 2024

subpop left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jirihnidek commented Dec 10, 2024

subpop left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

subpop left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

subpop left a comment

Choose a reason for hiding this comment

jirihnidek commented Nov 21, 2024 •

edited

Loading