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

Implement SASL2, BIND2, and FAST #1006

Closed
wants to merge 31 commits into from
Closed

Conversation

singpolyma
Copy link
Contributor

@singpolyma singpolyma commented Nov 21, 2023

Depends on #1030

@singpolyma singpolyma force-pushed the sasl2 branch 3 times, most recently from d655154 to 7c6e9c4 Compare November 22, 2023 21:28
@Neustradamus

This comment was marked as off-topic.

@sonnyp
Copy link
Member

sonnyp commented Dec 16, 2023

Thanks! I will get around to fixing CI and reviewing once I have a bit of time.

@singpolyma
Copy link
Contributor Author

Not to be a pain, but any chance to get this looked at? Thanks :)

Copy link
Member

@sonnyp sonnyp left a comment

Choose a reason for hiding this comment

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

Hey thanks for this!

could you add tests? check the other similar modules

e2e tests would be needed as well

CI is broken but I can have a look eventually - let me know if you have troubles running tests locally but a fix would be awesome

@singpolyma
Copy link
Contributor Author

@sonnyp tests added

Copy link
Member

@sonnyp sonnyp left a comment

Choose a reason for hiding this comment

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

I fixed CI and merged main into this. Plus a couple of minor changes, please see my commits

Please add end2end tests. See test/

You'll need to enable sasl2, bind2, fast in our Prosody (see server/) somehow so that tests that make use of it can be added.

End to end tests can be run with make ci

packages/sasl-ht-sha-256-none/index.js Outdated Show resolved Hide resolved
packages/sasl2/index.js Outdated Show resolved Hide resolved
}
}

module.exports = SASLError;
Copy link
Member

Choose a reason for hiding this comment

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

either this is different from SASLError or it uses the one from the @xmpp/saslerror package

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 don't see such a package?

Copy link
Member

Choose a reason for hiding this comment

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

sorry I meant form the @xmpp/sasl package

Comment on lines 9 to 11
if (!prevent && entity.jid) entity._status("online", entity.jid);
if (!prevent && entity.jid && entity.status !== "online") {
entity._status("online", entity.jid);
}
Copy link
Member

Choose a reason for hiding this comment

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

why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added comment to explain: BIND2 inline handler may have already set to online, eg inline SM resum

Comment on lines 261 to 267
if (
this.socket.secure &&
this.socket.secure() &&
(this.streamFrom || this.jid)
) {
headerElement.attrs.from = (this.streamFrom || this.jid).toString();
}
Copy link
Member

Choose a reason for hiding this comment

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

why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added comment to explain:
When the stream is secure there is no leak to setting the stream from
This is suggested in general and in required for FAST implementations
in particular

@Neustradamus
Copy link

@sonnyp: Thanks to work on this @singpolyma PR :)

No details in description but supported XEPs in this PR:

@singpolyma: XEPs are up-to-date?
Since one year, updates have been done at XSF about a lot of XEPs :/

It will be nice to add XEP supported versions (example: 1.0.0) to be listed correctly in xmpp.org and to update easily:

@singpolyma
Copy link
Contributor Author

Why removing the new sasl mechanism from browser? That's the main place I'm using it

@singpolyma
Copy link
Contributor Author

@sonnyp e2e tests added and they work locally and in CI (that one CI failure looks like a hiccup to me but I don't have permission to ask it to rerun I guess)

@Neustradamus
Copy link

@singpolyma: Have you tested with ejabberd?
It supports XEP-0386 and XEP-0388 since a moment.
And @Prefix has added XEP-0484 recently and will be in ejabberd 24.12.

cc: @mremond.

if csi_state_tag then
session.state = csi_state_tag.name;
end
end, 10);
Copy link
Member

Choose a reason for hiding this comment

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

aren't these available on prosody-trunk ?

I don't want to manage in tree prosody modules

Copy link
Member

Choose a reason for hiding this comment

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

we could add the prosody modules repo as a submodule to server

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Prosody modules repo is in mercurial not git. Could document what modules are needed and install them with the plugin installer CI or something maybe if you object to having local copies of the ones we need.

@@ -263,6 +263,9 @@ class Connection extends EventEmitter {
this.socket.secure() &&
(this.streamFrom || this.jid)
) {
// When the stream is secure there is no leak to setting the stream from
Copy link
Member

Choose a reason for hiding this comment

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

there is no leak to setting the stream from

Needs clarification

and a reference to the spec 🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What kind of clarification would you like for that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Spec reference:

However, if the client knows the XMPP identity then it SHOULD include the 'from' attribute after the confidentiality and integrity of the stream are protected via TLS or an equivalent security layer.
https://www.rfc-editor.org/rfc/rfc6120.html#section-4.7.1

packages/sasl2/test.js Show resolved Hide resolved
packages/sasl2/test.js Outdated Show resolved Hide resolved
packages/sasl2/README.md Outdated Show resolved Hide resolved
packages/sasl-ht-sha-256-none/index.js Outdated Show resolved Hide resolved
packages/sasl-ht-sha-256-none/index.js Outdated Show resolved Hide resolved

Mechanism.prototype.response = (cred) => {
this.password = cred.password;
const hmac = createHmac("sha256", this.password);
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't password and token be 2 different concepts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From the point of view of a sasl mechanism it's the same thing. It doesn't know if you're using a token or a password or what, it's just some shared secret string.

Comment on lines 267 to 268
// This is suggested in general and in required for FAST implementations
// in particular
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// This is suggested in general and in required for FAST implementations
// in particular
// This is recommended in general and required for FAST implementations

Copy link
Member

Choose a reason for hiding this comment

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

I don't see it in the spec

if (
this.socket.secure &&
this.socket.secure() &&
(this.streamFrom || this.jid)
Copy link
Member

Choose a reason for hiding this comment

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

why do we need a new streamFrom param?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because we want to set the from on initial connection, before the jid is otherwise known in a c2s case, so it must be provided by the caller. Setting the jid too early causes various parts of the library to assume we have done binding already since that is where that value comes from curretly.

@sonnyp sonnyp mentioned this pull request Dec 20, 2024
@sonnyp
Copy link
Member

sonnyp commented Dec 22, 2024

Update on xmpp.js

  1. migrated test framework from Ava to Jest
  2. migrated from commonjs to ESM (ECMAScript modules)
  3. replaced browserify with rollup (and shaved > 2kb out of the browser bundle)

Update on this PR

  1. merged main and updated to ESM/Jest
  2. replaced in-tree modules with prosodyctl install
  3. moved sasl factory from sasl/sasl2 to @xmpp/client so it can be shared

I'm now considering splitting this PR into multiple parts

@sonnyp sonnyp closed this Jan 5, 2025
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.

3 participants