GUACAMOLE-2300: Add IPMI Serial-over-LAN protocol support

[ciroiriarte]

This adds a new ipmi protocol module to guacamole-server providing Serial-over-LAN (SOL) console access and chassis power management for BMCs (baseboard management controllers), built on FreeIPMI (libipmiconsole for SOL, libfreeipmi for chassis control).

JIRA: https://issues.apache.org/jira/browse/GUACAMOLE-2300

What's included

• New libguac-client-ipmi protocol module — SOL console rendered through the shared guac_terminal.
• Chassis control over a client-agnostic ipmi-control pipe channel (newline-delimited JSON): power on/off/cycle/reset, soft shutdown, diagnostic interrupt (NMI), chassis identify, System Event Log (SEL) viewer, and live power/SOL status.
• In-terminal control menu (Ctrl+]) with an alternate-screen UI, run asynchronously so BMC round-trips never block the console.
• Multivendor support: configurable cipher suite / privilege level / workaround flags, hex K_g, encryption policy. Verified end-to-end against Supermicro (ATEN), Lenovo XCC, and Dell iDRAC 9.
• Connection parameters including persistent boot-device selection and keepalive interval.
• CUnit tests for the control-channel JSON parser, wired into make check.

Testing

• 12/12 unit tests pass.
• AddressSanitizer clean on live SOL sessions. Two memory bugs were found and fixed during review: a NULL-deref on teardown, and a use-after-free that freed the terminal before joining the SOL worker.
• Full-stack live regression (browser → guacamole-client → guacd → BMC): SOL console I/O, live status push, chassis identify, SEL read, and Send Break all verified non-destructively against a Supermicro BMC.

Related PRs

• Client: GUACAMOLE-2300: Add IPMI Serial-over-LAN protocol support guacamole-client#1228
• Manual: GUACAMOLE-2300: Add IPMI Serial-over-LAN protocol support guacamole-manual#298

[necouchman]

@ciroiriarte This looks really great, I'm excited to have this in the Guacamole code! I do have some initial comments - don't despair, most of these are style and documentation clean-up, although some deal with implementation details. Generally:

• In several places you've defined Guacamole-specific data structures or enums that mirror ones provided by the IPMI libraries. Is there any reason to do this? It seems like it would simplify things and cut down the amount of code to just reuse what is already there, unless there's a technical or implementation-specific reason why not?
• Make sure you've fully-documented all of the functions and/or function prototypes. There are several places where thinks like @param and @return tags are missing, and a handful where there was no documentation.
• There are a few places where I think some more verbose comments within the code would help people more easily understand what's being done and why.
• I see what you're doing with the IPMI menu, but I wonder if there's a way to use Guacamole's built-in menu to handle these functions rather than implementing the text-based menu you have and then having to implement the JSON-related code around it? I haven't looked at this in any detail, it just seems to me that that's what our existing hidden Guacamole menu is meant for, and there ought to be a way to handle things like chassis commands and the like similar to how we handle file transfers, terminal parameters, etc.

[necouchman]

Why are only string values allowed? My experience in general with JSON is that string values are expected for strings, but other values - boolean, numeric, etc. - are perfectly valid JSON?

[ciroiriarte]

guac_ipmi_control_json_get() bails if the value isn't quoted, so {"id":123} from a third-party client parses as absent. Given your point about independent client implementations, I'll extend it to accept numeric and boolean scalars.

[necouchman]

Mike recently submitted a PR that makes including config.h unnecessary - see #691.

[ciroiriarte]

Removed in 86f6677 — config.h is auto-included in every .c file since GUACAMOLE-2182, so the explicit include is redundant.

[necouchman]

Is there a particular reason we need to define new values for these privilege levels as GUAC_ constants and shouldn't just re-use the IPMI_PRIVILEGE_LEVEL_ values throughout the code?

[ciroiriarte]

Same privilege question — answered in full under the enum definition in settings.h (#692 (comment)). Short version: libipmiconsole and libfreeipmi disagree on the privilege values (0/1/2 vs 0x02/0x03/0x04), so reusing either is silently wrong on the other path. I'll keep an internal enum but give it arbitrary values and map explicitly on both paths.

[necouchman]

Similar to the privilege levels - is there a particular reason that we need to create and map them rather than just using the ones provided by FreeIPMI?

[ciroiriarte]

Different rationale from the privilege levels here, and a weaker one — I'd rather be upfront about that than stretch the same argument to cover it. There's no conflicting-constants problem for power actions or boot devices; these enums are less a wrapper around the FreeIPMI constants than internal command state. Each carries a NONE member that has no IPMI equivalent (IPMI_CHASSIS_CONTROL_* can't express "no action requested"), and they're used well away from this mapping — parsing the client's command strings, the menu's display labels, and the pending-confirmation state in guac_ipmi_client. The translation to the FreeIPMI constant happens only here, at the boundary.

NONE could be modelled as optional/nullable state instead, which would let these collapse into the raw constants. I lean toward keeping them as internal state enums for the parsing and menu use, but if you'd prefer the nullable approach I'll switch.

[necouchman]

And, similar to above two - why do we need to redefine these?

[ciroiriarte]

Same rationale as the power-action enum just above (#692 (comment)): these are internal command state with a NONE sentinel that has no IPMI_CHASSIS_CONTROL_* / boot-device equivalent, mapped to the FreeIPMI constant only here at the boundary.

[necouchman]

Please document parameters and return value.

[ciroiriarte]

Documented in 86f6677.

[necouchman]

Might be good to log this at WARN or DEBUG level?

[ciroiriarte]

Added a DEBUG log for this case in 86f6677.

[necouchman]

Please document each of the values. Also, as mentioned in one of the other files, is there any reason not to just use the IPMICONSOLE_PRIVILEGE_* values?? Why redefine them?

[ciroiriarte]

Two things here — the "why redefine" question and the request to document the values.

On reuse: this is the one enum I'd argue for keeping, because the module talks to two libraries whose privilege constants don't agree. SOL goes through libipmiconsole (IPMICONSOLE_PRIVILEGE_USER/OPERATOR/ADMIN = 0/1/2); chassis goes through libfreeipmi (IPMI_PRIVILEGE_LEVEL_USER/OPERATOR/ADMIN = 0x02/0x03/0x04). So "just reuse the library's values" is ambiguous, and a value picked for one library is silently wrong on the other path — an IPMICONSOLE_PRIVILEGE_ADMIN (2) reaching the chassis code reads as IPMI_PRIVILEGE_LEVEL_USER (0x02), a privilege downgrade with nothing to flag it.

The current code does invite the question fairly, though: the enum is defined as USER=0, OPERATOR=1, ADMIN=2 — hard-coded to the libipmiconsole numbering — and ipmi.c assigns it straight into ipmiconsole_ipmi_config.privilege_level with no mapping, while only the chassis side maps (via guac_ipmi_map_privilege_level()). So on the SOL path the enum really is just a copy of libipmiconsole's constants, which is why it reads as redundant. I'll give the values arbitrary numbering (not 0/1/2) and add a companion guac_ipmi_map_privilege_level_ipmiconsole() mirroring the chassis switch, so neither back-end ever receives the raw enum value.

To be straight about it: two conflicting constant sets don't prove a third enum is the only option — parsing directly to each library's constant at its boundary would work too. The internal enum just keeps the parsed setting semantic between the settings layer and the two back-ends instead of committing to one library's numbering at parse time. If you'd rather drop it and map at the boundary, I'll do that instead.

On the documentation: I'll add a doc comment to each value as part of that change.

[ciroiriarte]

Follow-up from above: this is now done in c29f725. The enum values are library-independent (INVALID = 0, then USER/OPERATOR/ADMIN), and the SOL path maps through a new guac_ipmi_map_privilege_level_ipmiconsole() mirroring the chassis-side guac_ipmi_map_privilege_level() — so neither back-end receives the raw enum value anymore.

[necouchman]

When you say "any other IPMI connections active within the same guacd process" - does this mean:

• Within the same guacd parent process?
• Within the same guacd child/connection process?

guacd forks a new process for each connection, and, since this particular lock would only be initialized in the context of a single connection, and that would be after the child connection has been forked by guacd, there actually should not be any issue with initializing it or tearing it down...

...UNLESS, you also mean that the IPMI chassis connections would interfere with the SOL connection, in which case, this would still make sense.

[ciroiriarte]

The comment is misleading, not just unclear. This lock has nothing to do with chassis-vs-SOL interference; that's handled separately by menu_lock/chassis_busy in control.c. All this guards is the refcount around ipmiconsole_engine_init()/ipmiconsole_engine_teardown(), and your reading of the fork model is the right one: guacd forks per connection, the refcount lives in that child, so it can only ever go 0→1 and the lock never contends. As written it's scaffolding for a sharing scenario that can't occur.

Let me simplify it rather than justify it. If it's worth keeping any of it, the argument would be that libipmiconsole's engine is process-global and the refcount is cheap insurance if guacd ever ran connections in threads rather than forks — but that's speculative, and I'd rather not carry speculative concurrency machinery. I'll cut it back to a plain init/teardown and fix the comment.

[necouchman]

Our practice throughout the code for static functions is generally just to implement them where they need to be in the code and not prototype them.

[ciroiriarte]

Done in 86f6677 — dropped the prototype and moved the definition ahead of its first use.

[ciroiriarte]

Thanks again for the thorough review, @necouchman — I've pushed 86f6677 addressing the feedback, and replied inline on each thread. A summary of what changed:

Documentation

• Completed @param/@return across control.c and settings.c, documented the workaround struct members and each enum value, and removed the doc blocks that duplicated control.h.

Style

• Dropped the now-redundant config.h includes (auto-included since GUACAMOLE-2182), one case label per line, moved file-level #define/typedef declarations to the top of chassis.c, and inlined the menu.c static function instead of forward-declaring it.

Settings parsers

• The privilege / power-action / boot-device / encryption-policy parsers are now pure string-to-enum converters — no logging, no defaults. Each returns a new *_INVALID sentinel and the default plus warning is applied by the caller (this also fixed a small leak where the parsed argument strings were never freed). Collapsed the administrator alias to admin.
• Removed the unused port setting entirely — both libipmiconsole and libfreeipmi hardwire 623/UDP, so it was read but never honored.

Behavior (discussed inline)

• Accept non-string JSON scalars in the control parser, return the power state directly from guac_ipmi_chassis_status(), reject unknown control commands explicitly, and drop the vestigial engine reference count/lock.

Still to come, from the enum discussion above: giving the privilege enum library-independent values and adding the symmetric guac_ipmi_map_privilege_level_ipmiconsole() mapper so neither back-end receives the raw enum value. I'll follow up with that shortly. Happy to iterate on anything here.