Skip to content

Adding support for ARD AuthType 33/35/36#698

Open
Chapoly1305 wants to merge 12 commits intoLibVNC:masterfrom
Chapoly1305:ard-auth-pr
Open

Adding support for ARD AuthType 33/35/36#698
Chapoly1305 wants to merge 12 commits intoLibVNC:masterfrom
Chapoly1305:ard-auth-pr

Conversation

@Chapoly1305
Copy link

This pull request introduces support for Apple Remote Desktop (ARD) authentication schemes and adds new cryptographic functionality to the codebase (to support this PR). The changes include updates to the build system for OpenSSL support on macOS, new ARD auth configuration options in the client, and implementations for key cryptographic primitives such as SHA-512, PBKDF2-HMAC-SHA512, and RSA encryption. The client authentication selection logic has been refactored to support multiple ARD schemes and prioritize user preferences.

ARD Authentication Support

  • Added new ARD authentication schemes (rfbARDAuthDH, rfbARDAuthRSASRP, rfbARDAuthKerberosGSSAPI, rfbARDAuthDirectSRP) and logic to select and handle them in the client, including ardauth.h and integration in rfbclient.c. [1] [2] [3] [4] [5] [6] [7] [8] [9]
  • Extended the rfbClient struct with optional ARD Kerberos authentication configuration fields and added setter functions for these fields. [1] [2]

Cryptography Enhancements

  • Added SHA-512 hash, PBKDF2-HMAC-SHA512 key derivation, and RSA PKCS#1 encryption primitives to the cryptographic API, with implementations for OpenSSL and Libgcrypt. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10]

Build System and Platform Support

  • Updated CMakeLists.txt to build crypto with OpenSSL on macOS, link ARD authentication sources, and add macOS-specific frameworks for the VNC client. [1] [2] [3]
  • Added new example and test targets for ARD authentication probing in the build system. [1] [2]

These changes collectively enable ARD authentication in the VNC client, provide the necessary cryptographic primitives, and ensure proper build and linking, especially for macOS platforms.

Copilot AI review requested due to automatic review settings March 20, 2026 21:21
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds Apple Remote Desktop (ARD) authentication support to libvncclient (auth types 30/33/35/36) and introduces new crypto primitives/build plumbing needed to implement those schemes (notably SHA-512, PBKDF2-HMAC-SHA512, and RSA PKCS#1 encryption).

Changes:

  • Implement ARD authentication handlers (DH, RSA-SRP, Kerberos GSS-API, Direct SRP) and refactor client auth-type selection to support them.
  • Extend the public client API with optional ARD Kerberos configuration fields + setters.
  • Add crypto primitives across OpenSSL/Libgcrypt backends and update CMake to prefer OpenSSL crypto on macOS and link required Apple frameworks; add an ARD probe example.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
test/ardauthprobe.c Adds a headless probe client for forcing/testing a specific ARD auth type.
src/libvncclient/vncviewer.c Initializes/frees new ARD Kerberos configuration fields in rfbClient lifecycle.
src/libvncclient/rfbclient.c Adds ARD auth support hooks, auth-type selection updates, and new public setters.
src/libvncclient/ardauth.h Declares the ARD auth handler entry point.
src/libvncclient/ardauth.c Implements ARD DH / RSA-SRP / Kerberos GSS-API / Direct SRP flows.
src/common/crypto_openssl.c Adds SHA-512, PBKDF2-HMAC-SHA512, and RSA SPKI DER encryption via OpenSSL.
src/common/crypto_libgcrypt.c Adds SHA-512 + PBKDF2-HMAC-SHA512 via Libgcrypt (RSA encrypt currently stubbed).
src/common/crypto_included.c Adds stubs for the new crypto APIs when crypto is not available.
src/common/crypto.h Exposes new crypto APIs/constants (SHA512, PBKDF2, RSA encrypt).
include/rfb/rfbproto.h Introduces new ARD auth type constants (replacing the old rfbARD macro).
include/rfb/rfbclient.h Extends rfbClient struct + declares new ARD Kerberos setter APIs.
CMakeLists.txt Prefers OpenSSL crypto on macOS, links ardauth.c, adds Apple frameworks, and builds the probe example.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@Chapoly1305 Chapoly1305 marked this pull request as draft March 20, 2026 22:18
@Chapoly1305 Chapoly1305 marked this pull request as ready for review March 20, 2026 22:35
Copy link
Member

@bk138 bk138 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

General thought: keep it as simple as possible, if we cannot support a functionality for the other platforms, leave it out :-)


/* flag to indicate wheter updateRect is managed by lib or user */
rfbBool isUpdateRectManagedByLib;
/* flag to indicate wheter updateRect is managed by lib or user */
Copy link
Member

Choose a reason for hiding this comment

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

please don't add unrelated whitespace changes

extern rfbBool ConnectToRFBServer(rfbClient* client,const char *hostname, int port);
extern rfbBool ConnectToRFBRepeater(rfbClient* client,const char *repeaterHost, int repeaterPort, const char *destHost, int destPort);
extern void SetClientAuthSchemes(rfbClient* client,const uint32_t *authSchemes, int size);
extern rfbBool rfbClientSetARDAuthRealm(rfbClient *client, const char *realm);
Copy link
Member

Choose a reason for hiding this comment

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

please doc-comment these

#define rfbTLS 18
#define rfbVeNCrypt 19
#define rfbSASL 20
#define rfbARD 30
Copy link
Member

Choose a reason for hiding this comment

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

don't change the API here.

Copy link
Author

Choose a reason for hiding this comment

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

I have thought about this. The issue is that ARD is the name for the full protocol, but authType 30 is just one of several authentication methods. Since this PR aims to add support for more authentication types, I think maybe this adjustment is worth considering.

#define rfbSASL 20
#define rfbARD 30
#define rfbARDAuthDH 30
#define rfbARDAuthRSASRP 33
Copy link
Member

Choose a reason for hiding this comment

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

please add comments here what these are

* -auth 35 ARD Kerberos GSS-API
* -auth 36 ARD Direct SRP
*
* Kerberos sample:
Copy link
Member

Choose a reason for hiding this comment

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

Kind of important question: are all these auth methods readily configured out of the box on a MacOS host? If not; I see two options:

  • omit them from this PR for the sake of simplicity
  • add setup instructions for the server here

Copy link
Author

Choose a reason for hiding this comment

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

"are all these auth methods readily configured out of the box on a MacOS host?"

Yes, I tested on macOS 15.7, and Type 30, 33, 35, and 36 are supported out of the box. No additional configuration is required for the server side. The client can directly authenticate with the server. Type 30, 33, and 36 only require a username and password, while Type 35 requires additional information from the keychain. I’m not sure where this information (for 35) comes from, but it’s there.

@@ -0,0 +1,152 @@
/**
* @example ardauthprobe.c
Copy link
Member

Choose a reason for hiding this comment

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

maybe this should go to examples/client/ rather as it's not really a test that can be automated, what do you think?

switch (authScheme) {
case rfbARDAuthDH:
return TRUE;
#if defined(__APPLE__)
Copy link
Member

Choose a reason for hiding this comment

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

It is a must that functionalitly we add works on every supported platform. If it does not, simply omit it :-)

@Chapoly1305
Copy link
Author

Thank you so much for taking the time to review the PR. I’ll carefully review the comments and make the necessary changes within the next week(s).

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.

3 participants