-
Notifications
You must be signed in to change notification settings - Fork 6
Add Session Pro as a concept to libsession #63
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
base: dev
Are you sure you want to change the base?
Conversation
This simplifies the API and minimises the API surface with code interacting with it since the public key is already bundled with the secret. - Add helper function to derive keys from the original seed - Use _hex_u literals to simplify test construction code
Design of the interface should be accomodating not opionated. This also fixes a bug where the pro backend key wasn't been initialised yet in the C interface
Due to BT encoding using sort order to order keys. Version at top means we can conditionally handle version changes easily.
include/session/session_protocol.hpp
Outdated
enum class EnvelopeType { | ||
SessionMessage = ENVELOPE_TYPE_SESSION_MESSAGE, | ||
ClosedGroupMessage = ENVELOPE_TYPE_CLOSED_GROUP_MESSGE, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't actually support sending messages to legacy closed groups anymore on any of the clients (iOS has removed most of the code to handle receiving these messages and I think Android will be doing the same soon) so you could potentially remove EnvelopeType::ClosedGroupMessage
since it was only used for legacy groups
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually it looks like we are still setting this for updated groups 🤔
Confirmed with the Android team - unfortunately they are still using it at the moment so we will need to keep it to avoid breaking old Android versions 🥲
src/session_protocol.cpp
Outdated
// Parse type (unconditionallty) | ||
if (!envelope.has_type()) | ||
throw std::runtime_error("Parse envelope failed, missing type"); | ||
|
||
switch (envelope.type()) { | ||
case SessionProtos::Envelope_Type_SESSION_MESSAGE: | ||
result.envelope.type = EnvelopeType::SessionMessage; | ||
break; | ||
|
||
case SessionProtos::Envelope_Type_CLOSED_GROUP_MESSAGE: | ||
result.envelope.type = EnvelopeType::ClosedGroupMessage; | ||
break; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think Android is the only one doing this now - just wondering whether we should properly deprecate this value and rely on the namespace that the message was received from (if a message with the wrong encryption type is stored in the wrong namespace then it should be considered invalid and dropped anyway)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For sending we still need to as above for android but for decrypting on receive, you're saying remove this branch and replace it with:
if (space == config::Namespace::GroupMessages) {
result.envelop.type = EnvelopeType::GroupMessage;
} else {
result.envelope.type = EnvelopeType::SessionMessage;
}
Is that right, where the C++ side still wants to keep the envelop type so application code after decryption has all necessary information self-contained in struct Envelope
to work with, without requiring any external context. But requiring the type
on the protobuf Envelope is no longer needed (and an outdated practice).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea that was my thinking - since the decryption code is based on the namespace
then we could eventually stop sending the type
entirely once enough time has passed
Is that right, where the C++ side still wants to keep the envelop type so application code after decryption has all necessary information self-contained in struct Envelope to work with, without requiring any external context.
I get where this is coming from but since the Envelope
is encrypted for messages sent to a group the decryption logic is already dependent on knowing the namespace
that a message came from which makes having a Envelope.type
feel redundant 😕 (ie. at this point we already know that it's a GroupMessage
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright. understood. I have removed the parsing of envelope types on decrypt.
include/session/session_protocol.hpp
Outdated
/// Both legacy and non-legacy closed groups are to be identified as `ClosedGroup`. A non-legacy | ||
/// group is detected by the (0x03) prefix byte on the given `dest_closed_group_pubkey` | ||
/// specified in Destination. | ||
ClosedGroup, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
again, legacy groups have been readonly for a while. Sending or receiving a message to/from it should never be done anymore.
struct Envelope { | ||
ENVELOPE_FLAGS flags; | ||
EnvelopeType type; | ||
std::chrono::milliseconds timestamp; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we should make this timestamp_ms
, just in case 👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have the suffix for the C API but C++'s type safety here makes it pretty hard to misuse the timestamp here in a context that isn't compatible with milliseconds.
include/session/session_protocol.hpp
Outdated
// Status flag for validity of the Session Pro proof embedded in the envelope if it has one. | ||
// The status is set to `Nil` if there is no Session Pro proof in the message. Otherwise it's | ||
// set to one of the other values to which the remaining pro fields will be populated with data | ||
// parsed from the envelope. | ||
ProStatus pro_status; | ||
|
||
// The embedded Session Pro proof, only set if the status was not `Nil`. | ||
config::ProProof pro_proof; | ||
|
||
// Session Pro bit flag features that were used in the embedded message, only set if the status | ||
// was not `Nil`. | ||
PRO_FEATURES pro_features; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it make sense to also have pro_features
unset if the pro_proof
is not Valid
?
It might be the case already, but if yes it would be good to have the comment updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to preserve the information so that you have the option of using diagnostics on the pro features that were requested but don't have a valid proof anymore. Similarly I don't wipe the pro proof data as I envision basically all client code should be branching on the status before applying pro features.
if (pro_status == ProStatus::Valid) {
// Do sensitive operations w/ pro features,
} else {
if (pro_features) { /*maybe diagnostics on failure*/ }
}
enum class SessionIDPrefix { | ||
standard = 0, | ||
group = 0x3, | ||
community_blinded_legacy = 0x5, | ||
community_blinded = 0x15, | ||
version_blinded = 0x25, | ||
unblinded = 0x7, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am very confused by this. Is it supposed to map to real prefixes?
If yes this should all be changed a fair bit:
enum class SessionIDPrefix { | |
standard = 0, | |
group = 0x3, | |
community_blinded_legacy = 0x5, | |
community_blinded = 0x15, | |
version_blinded = 0x25, | |
unblinded = 0x7, | |
}; | |
enum class SessionIDPrefix { | |
standard = 0x05, | |
group = 0x03, | |
community_unblinded = 0x05, | |
community_blinded_legacy = 0x15, | |
community_blinded = 0x25, | |
version_blinded = 0x07, | |
}; |
I guess 0x3 is actually 0x03, but the prefixes/names associated are not correct too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The community_unblinded
uses a 00
prefix which represents the users ed25519 pubkey (and a signature signed with that) whereas the 05
prefix would be for the users x25519 pubkey (and it's signature)
It's used for the SOGS signature header when blinding is disabled - for reference there is a function to get the string prefix for this enum in internal.hpp
:
inline constexpr std::string_view to_string(session::SessionIDPrefix prefix) {
switch (prefix) {
case session::SessionIDPrefix::unblinded: return "00"sv;
case session::SessionIDPrefix::group: return "03"sv;
case session::SessionIDPrefix::standard: return "05"sv;
case session::SessionIDPrefix::community_blinded_legacy: return "15"sv;
case session::SessionIDPrefix::community_blinded: return "25"sv;
case session::SessionIDPrefix::version_blinded: return "07"sv;
}
return "05"sv; // Fallback to standard, shouldn't occur
};
src/session_protocol.cpp
Outdated
// Legacy closed groups which have a 05 prefixed key | ||
enc.mode = Mode::Envelope; | ||
enc.before_envelope_encrypt_for_recipient_deterministic = true; | ||
enc.envelope_type = | ||
SessionProtos::Envelope_Type::Envelope_Type_CLOSED_GROUP_MESSAGE; | ||
enc.after_envelope = AfterEnvelope::WrapInWSMessage; | ||
enc.envelope_src = dest_closed_group_pubkey; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to me, this should throw
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need confirmation that this branch can't get hit: https://github.com/session-foundation/session-ios/blob/82deef869d0f7389b799295817f42ad14f8a1316/SessionMessagingKit/Sending%20%26%20Receiving/MessageSender.swift#L425
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The branch you've linked is the encryption for updated groups, legacy groups would fall into this case: https://github.com/session-foundation/session-ios/blob/82deef869d0f7389b799295817f42ad14f8a1316/SessionMessagingKit/Sending%20%26%20Receiving/MessageSender.swift#L449
The code can be hit by a modified client at the moment, but most of the legacy groups logic has been removed so while a VisibleMessage
would probably work, anything needed to update a legacy group state would fail - on a non-modified client legacy groups are in a read-only state so there shouldn't be a way to send a message (ie. throwing in this case should be fine)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, updated, throws if you try and hit that codepath.
if (envelope.has_servertimestamp()) { | ||
result.envelope.server_timestamp = envelope.servertimestamp(); | ||
result.envelope.flags |= ENVELOPE_FLAGS_SERVER_TIMESTAMP; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not think this is anything used by any platforms, but to verify.
src/session_protocol.cpp
Outdated
// A signature must always be present on the envelope. This is to make a pro and non-pro | ||
// envelope indistinguishable. If the message does not have pro then this signature must still | ||
// be set but will be ignored. So in all instances a signature must be attached (real or | ||
// dummy). | ||
if (!envelope.has_prosig()) | ||
throw std::runtime_error("Parse envelope failed, message is missing pro signature"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't do this, yet. This would throw for any messages already stored in the swarm, and until everyone has upgraded their app, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is correct, I have fixed it by making it optional now. Added a note that we can enforce it in a year, but for now we can't otherwise it'd break all the old clients as you have mentioned.
This lets the higher-level layer, session protocol to use low level encryption instead of relying on config to decrypt group messages. Decryption of a group message is a low-level cryptography routine which actually belongs in session encrypt.
We still require it to be optional whilst old clients are sending envelopes without the signature attached (real or dummy)
constexpr array_uc32 PUBKEY = {0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, | ||
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, | ||
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Appears unused (aside from a static assert, which could simply be changed to sizeof(array_uc32) == ...
or array_uc32{}.size() == ...
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the static assert and I've added a comment as to what its purpose is. It's meant to be set to the actual Session Pro backend key so that eventually users of the library have the key handy in the interface so as to when they go and verify the proofs they can just pull it from the header.
For libsession I've made the decrypt and verify function parameterise the Session Pro backend public key that it accepts so that you can use it against any arbitrary key (so that it's useful for tests) instead of baking in the key from the header. But the idea is that you can use the key in this header officially for that purpose.
The C-API is a wrapper over the C-interface so it thunks into the C++ implementation anyway so it tests both the C and C++ api for us with a very few exceptions. But for the most part, testing the C API kills two birds with one stone and reduces the test maintenance overhead of having to manage 2 test suites in parallel for the same API.
All the payloads in the returned envelope are decrypted so we shouldn't care too much about the type at that point. The caller can immediately start working with the contents.
include/session/session_protocol.hpp
Outdated
enum class ProStatus { | ||
// Proof not set | ||
Nil = PRO_STATUS_NIL, | ||
// Proof set; pro proof sig was not produced by the Pro backend key | ||
InvalidProBackendSig = PRO_STATUS_INVALID_PRO_BACKEND_SIG, | ||
// Proof set; envelope pro sig was not produced by the Rotating key | ||
InvalidUserSig = PRO_STATUS_INVALID_USER_SIG, | ||
// Proof set, is verified; has not expired | ||
Valid = PRO_STATUS_VALID, | ||
// Proof set, is verified; has expired | ||
Expired = PRO_STATUS_EXPIRED, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we leave the expired state out of this status? Whether a pro has expired depends on current time, which I find it the best to leave it to the caller to decide - so that:
- They can supply their own clock
- They know when to recheck for validity.
For this reason I suggest we do these states instead:
enum class ProStatus { | |
// Proof not set | |
Nil = PRO_STATUS_NIL, | |
// Proof set; pro proof sig was not produced by the Pro backend key | |
InvalidProBackendSig = PRO_STATUS_INVALID_PRO_BACKEND_SIG, | |
// Proof set; envelope pro sig was not produced by the Rotating key | |
InvalidUserSig = PRO_STATUS_INVALID_USER_SIG, | |
// Proof set, is verified; has not expired | |
Valid = PRO_STATUS_VALID, | |
// Proof set, is verified; has expired | |
Expired = PRO_STATUS_EXPIRED, | |
}; | |
enum class ProStatus { | |
// Proof not set | |
Nil = PRO_STATUS_NIL, | |
// Proof set; pro proof sig was not produced by the Pro backend key | |
InvalidProBackendSig = PRO_STATUS_INVALID_PRO_BACKEND_SIG, | |
// Proof set; envelope pro sig was not produced by the Rotating key | |
InvalidUserSig = PRO_STATUS_INVALID_USER_SIG, | |
// Proof set, is verified; whether it's expired you should check the expiration time in pro_proof | |
Verified = PRO_STATUS_VERIFIED, | |
}; |
Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this is currently the case that the caller is in control of the clock.
/// - `unix_ts` -- pass in the current system time in seconds which is used to determine, whether or
/// not the Session Pro proof has expired or not if it is in the payload. Ignored if there's no
/// proof in the message.
DecryptedEnvelope decrypt_envelope(
const DecryptEnvelopeKey& keys,
std::span<const uint8_t> envelope_payload,
std::chrono::sys_seconds unix_ts,
const array_uc32& pro_backend_pubkey);
Pro verification in regards to extracting a pro status from the proof is currently embedded in the decryption function but I intend to pull that out to some utility functions some point later so you can call it standalone. I plan to add those after if we're generally happy with the approach here before writing all the helper utilities that go ontop.
Is that sufficient for your use case? Or would you still want to separate the expired-ness from PRO_STATUS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use case is when decrypt_envelop
is called, the user hasn't expired yet, let's say 10s later it expires, we won't be calling the decrypt_envelop
to determine the validity again, we (the application code) will have to arrange its own timer to check the expire timestamp - hence it's best not for the libsession to say whether the pro has expired as it will be tempting to think that as long as the result you get is valid, it's valid forever. Just pushing the expiration check's responsibility to the user forces them to handle it correctly.
This is more about making the API hard to misuse, as I believe at the current form, all the data the application needs is right there. In terms of extracting a pro status from the proof - I think I have some other ideas on the DecryptedEnvelop
so let's see how you think of that suggestion instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After a thought I think it doesn't make sense to not have libsession validating the expired status, "expiration" after all, is a criteria of validity. So forget about this one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, yes I think so we can move expiry out and not merge it into the status field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has been handled out-of-band, we've decided to keep expired in the status, but, reaffirmed that it's useful to have that separated as the clients envision having to check expiry themselves as well independently from decryption at later points.
include/session/session_protocol.hpp
Outdated
// Status flag for validity of the Session Pro proof embedded in the envelope if it has one. | ||
// The status is set to `Nil` if there is no Session Pro proof in the message. Otherwise it's | ||
// set to one of the other values to which the remaining pro fields will be populated with data | ||
// parsed from the envelope. | ||
ProStatus pro_status; | ||
|
||
// The embedded Session Pro proof, only set if the status was not `Nil`. | ||
config::ProProof pro_proof; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we have a union type for these instead? So you won't have to worry "only set if status was not Nil":
struct Nil {};
struct InvalidSig {
PRO_FEATURES pro_features_requested;
};
struct InvalidBackendSig : InvalidSig {};
struct InvalidUserSig: InvalidSig {};
struct Valid {
config::ProProof pro_proof;
PRO_FEATURES pro_features;
};
typedef std::variant<Nil, InvalidBackendSig, InvalidUserSig, Valid> ProStatus;
This ProStatus
will then be the only property you need on the DecryptedEnvelop
, and you won't be able to get to any invalid state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handled out-of-band, the solution is to embed the PRO_STATUS into a std::optional<ProProofWrapper>
-esque structure so that the presence of the optional determines if pro is nil or non-nil.
User code then just needs to branch on the optional to know that there's some, maybe invalid, pro data to work with
std::optional<ProProofWrapper> pro_proof_wrapper;
if (pro_proof_wrapper) {
assert(pro_proof_wrapper->pro_status != NIL); // Cannot be nil by definition because the optional is present. So nil state will be removed from the set of possibilities
}
So that removes ProStatus::Nil from the enum and solves the having to worry about a nil status. That is implicit in the presence of the optional
Proofs that are signed by the Session Pro backend are now present in the library as a concept. It's a class that contains the fields that the Session Pro Backend will produce that is encapsulated in
ProProof
. Proofs can be verified and validated and embedded into messages. This PR goes ontop of: #61The bulk of this code is fulfilling the requirement
This main functionality is in a new file
session_protocol.cpp
whose intent is to be high level wrappers over the lower level cryptography insession_encrypt.cpp
. The main work horse functions areEncrypting and decrypt the envelope was implemented by referencing the following implementations across the platforms:
iOS:
Desktop: https://github.com/session-foundation/session-desktop/blob/4f51c28cc0ba999df0b46eb09d83bdfa9b372ab5/ts/session/sending/MessageQueue.ts
Android: https://github.com/session-foundation/session-android/blob/e4c16ae8aa8cae2f75aba565b890082563b07bd2/app/src/main/java/org/session/libsession/messaging/sending_receiving/MessageSender.kt#L398
Updating protobufs to include the new pro metadata, the main outline of those changes:
--
The unit tests have examples of how to use the API. The idea here is that the platforms will fill in the pro metadata if they have some into the protobufs. You can then hand over the serialised protobuf
plaintext
for encryption by configuring theDestination
as to where the message is intended to go (1o1, legacy groups or groups v2 essentially) with the necessary keys and callencrypt_for_destination
.You then call
decrypt_envelope
when you receive a payload from the wire to get the plaintext content as well as the pro metadata that was available in that message.Libsession will do the encryption and return the final payload suitable for sending on the wire. The reverse is available to decrypt an envelope. It supports decrypting both an envelope that is encrypted (e.g.: A groups v2 envelope) and an unencrypted envelope (e.g.: 1o1 and legacy groups envelope) where the content is instead encrypted with the recipient keys.
Still todo:
Content
on the wire directly) to enable pro features in those message in a way that is forwards and backwards compatible.All the additional helper functions required in the Session Pro PRD (e.g. separating out PRO verification functions so you can call it standalone and evaluate current device PRO status, e.t.c)