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

feat(agent): Devolutions agent bootstrap #875

Merged
merged 3 commits into from
Jun 4, 2024
Merged

Conversation

pacmancoder
Copy link
Contributor

@pacmancoder pacmancoder commented Jun 1, 2024

This PR bootstraps the Devolutions Agent project.
Although this PR is huge, most of it is because of copy-paste from Devolutions Gateway MSI, so don't be discouraged -- I left comments in code where it really requires review

The overall list of changes:

  • @awakecoding added skeleton for devolutions-agent crate
  • Added config and logging for devolutions-agent
  • MSI package for Devolutions Agent
    • Copied from Gateway, removed all unnecessary code/dialogs/strings for Agent
    • May require follow-up PR to reduce code duplication between Agent/Gateway (e.g. for helper classes)
  • Debian package for Devolutions Agent (though, it requires testing)
  • tlk.ps1 now builds both Gateway and Agent
    • Added -Product parameter to select product to build/package
  • Modified GitHub actions CI scripts to build Agent alongside Gateway

MSI installer presentation:

2024-06-01.15-54-50.mp4

Comment on lines +18 to +22
pub trait StaticLogConfig {
const MAX_BYTES_PER_LOG_FILE: u64;
const MAX_LOG_FILES: usize;
const LOG_FILE_PREFIX: &'static str;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only change in this file after move is making some logging parameters generic via trait

/// Unstable options are subject to change
#[derive(PartialEq, Debug, Clone, Serialize, Deserialize)]
#[serde(rename_all = "PascalCase")]
pub struct ConfFile {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copied from Gateway, kept only fields for logging

Comment on lines +105 to +108
let subcommand = env::args().nth(2).expect("missing config subcommand");
if let Err(e) = config::handle_cli(subcommand.as_str()) {
eprintln!("[ERROR] Agent configuration failed: {}", e);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

simple config initialization logic to call from MSI

@@ -0,0 +1,251 @@
using System;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Service, WinAPI, and other helpers (which were copied without change except namespace from Gateway) could be refactored and moved to a separate "Common" project to share between Agent and Gateway, but I think this could be done as a follow-up


internal static IEnumerable<Type> Dialogs => Sequence;

internal static int Move(IManagedDialog current, bool forward)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed (currently not required) configuration logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't had much experience with PowerShell still, @awakecoding please take a look at these changes 👀

In short -- I have added new -Product <product> parameter to tlk.ps1 and added code to build/package Devolutions Agent on Windows & Linux

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mostly copied from Gateway, removed all tasks except log deleter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Kept only minimal amount of actions, removed everything gateway-related

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All dialog logic were kept unchanged (only strings changed)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not a designer by any means, but I made temporary changes to icon/banners just to easily distinguish between gateway and agent installers

Copy link
Member

@CBenoit CBenoit Jun 1, 2024

Choose a reason for hiding this comment

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

question: Are you sure we need a separate .gitignore in this folder? target/ and Cargo.lock is supposed to be shared with the rest of the workspace.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, it indeed should be removed 👍

Comment on lines +1 to +2
# devolutions-log
Common code for logging in devolutions apps
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: Sorry for repeating myself, but to strictly it’s only for the services provided in this repository, and we do have other services that this crate is not supposed to be used for. A different wording could be used. But not really a big deal.

Copy link
Member

@CBenoit CBenoit left a comment

Choose a reason for hiding this comment

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

Very good job!! I think it’s looking good so I’ll approve. But it could be nice to wait for input from @thenextman on the installer. Also, you probably want to remove the extra .gitignore which was likely committed by accident.

@pacmancoder
Copy link
Contributor Author

@thenextman Could you please skim over the MSI code?

@awakecoding awakecoding merged commit 38d27b4 into master Jun 4, 2024
25 of 38 checks passed
@awakecoding awakecoding deleted the devolutions-agent branch June 4, 2024 20:27
/// BUILTIN\Administrators Allow FullControl
/// BUILTIN\Users Allow ReadAndExecute, Synchronize
/// </remarks>
internal static string PROGRAM_DATA_SDDL = "O:SYG:SYD:PAI(A;OICI;FA;;;SY)(A;OICI;0x1201bf;;;LS)(A;OICI;0x1301bf;;;NS)(A;OICI;FA;;;BA)(A;OICI;0x1200a9;;;BU)";
Copy link
Member

Choose a reason for hiding this comment

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

NETWORK SERVICE can be removed from the permission set here and below, I'd add a comment to point out that this differs from the Gateway install.

I found the easiest way to generate the SDDL string is to configure the permissions you want manually, then use PowerShell (Get-ACL has a SDDL property IIRC)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll fix this in follow-up, thanks!

@thenextman
Copy link
Member

@thenextman Could you please skim over the MSI code?

I skimmed and made a single comment, in the future it will be nice to consolidate the shared code but as you wrote: looks good for now :)

Can we check that the Package.yml workflow didn't get any breaking changes before merging?

@pacmancoder
Copy link
Contributor Author

Can we check that the Package.yml workflow didn't get any breaking changes before merging

The branch was already merged a few days ago, and the packaging task was broken and then fixed by #879 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants