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

Basic EXTJWT support #948

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions conventional.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,17 @@ server:
# - "192.168.1.1"
# - "192.168.10.1/24"

# these services can integrate with the ircd using JSON Web Tokens (https://jwt.io)
# sometimes referred to with 'EXTJWT'
jwt-services:
# # service name
# call-host:
# # custom expiry length, default is 30s
# expiry-in-seconds: 45

# # secret string to verify the generated tokens
# secret: call-hosting-secret-token

# allow use of the RESUME extension over plaintext connections:
# do not enable this unless the ircd is only accessible over internal networks
allow-plaintext-resume: false
Expand Down
2 changes: 1 addition & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,10 @@ go 1.14

require (
code.cloudfoundry.org/bytefmt v0.0.0-20200131002437-cf55d5288a48
github.com/dgrijalva/jwt-go v3.2.0+incompatible
github.com/docopt/docopt-go v0.0.0-20180111231733-ee0de3bc6815
github.com/go-ldap/ldap/v3 v3.1.7
github.com/go-sql-driver/mysql v1.5.0
github.com/goshuirc/e-nfa v0.0.0-20160917075329-7071788e3940 // indirect
github.com/goshuirc/irc-go v0.0.0-20200311142257-57fd157327ac
github.com/onsi/ginkgo v1.12.0 // indirect
github.com/onsi/gomega v1.9.0 // indirect
Expand Down
2 changes: 2 additions & 0 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ code.cloudfoundry.org/bytefmt v0.0.0-20200131002437-cf55d5288a48/go.mod h1:wN/zk
github.com/DanielOaks/go-idn v0.0.0-20160120021903-76db0e10dc65/go.mod h1:GYIaL2hleNQvfMUBTes1Zd/lDTyI/p2hv3kYB4jssyU=
github.com/davecgh/go-spew v1.1.0 h1:ZDRjVQ15GmhC3fiQ8ni8+OwkZQO4DARzQgrnXU1Liz8=
github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
github.com/dgrijalva/jwt-go v3.2.0+incompatible h1:7qlOGliEKZXTDg6OTjfoBKDXWrumCAMpl/TFQ4/5kLM=
github.com/dgrijalva/jwt-go v3.2.0+incompatible/go.mod h1:E3ru+11k8xSBh+hMPgOLZmtrrCbhqsmaPHjLKYnJCaQ=
github.com/docopt/docopt-go v0.0.0-20180111231733-ee0de3bc6815 h1:bWDMxwH3px2JBh6AyO7hdCn/PkvCZXii8TGj7sbtEbQ=
github.com/docopt/docopt-go v0.0.0-20180111231733-ee0de3bc6815/go.mod h1:WwZ+bS3ebgob9U8Nd0kOddGdZWjyMGR8Wziv+TBNwSE=
github.com/fsnotify/fsnotify v1.4.7 h1:IXs+QLmnXW2CcXuY+8Mzv/fWEsPGWxqefPtCP5CnV9I=
Expand Down
33 changes: 28 additions & 5 deletions irc/channel.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ type Channel struct {
key string
members MemberSet
membersCache []*Client // allow iteration over channel members without holding the lock
memberJoinTimes map[*Client]time.Time
Copy link
Member

Choose a reason for hiding this comment

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

Ugh --- there must be some reason this is necessary, but I can't figure out what it is yet. I asked a question about this on the spec PR. It would nice if we didn't need this.

Copy link
Member

@slingamn slingamn Apr 17, 2020

Choose a reason for hiding this comment

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

I discussed this with prawn and it sounds like we can just send 1 as the join timestamp (indicating "the client is joined, but the join timestamp is unavailable"); we can probably wait a couple days and then remove this tracking.

Copy link
Member Author

Choose a reason for hiding this comment

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

Once the spec is updated we can handle this however I guess, no worries

Copy link
Member

Choose a reason for hiding this comment

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

The spec got updated to allow 1 here.

name string
nameCasefolded string
server *Server
Expand Down Expand Up @@ -62,11 +63,12 @@ func NewChannel(s *Server, name, casefoldedName string, registered bool) *Channe
config := s.Config()

channel := &Channel{
createdTime: time.Now().UTC(), // may be overwritten by applyRegInfo
members: make(MemberSet),
name: name,
nameCasefolded: casefoldedName,
server: s,
createdTime: time.Now().UTC(), // may be overwritten by applyRegInfo
members: make(MemberSet),
memberJoinTimes: make(map[*Client]time.Time),
name: name,
nameCasefolded: casefoldedName,
server: s,
}

channel.initializeLists()
Expand Down Expand Up @@ -539,6 +541,25 @@ func (channel *Channel) ClientPrefixes(client *Client, isMultiPrefix bool) strin
}
}

func (channel *Channel) ClientModeStrings(client *Client) (result []string) {
channel.stateMutex.RLock()
defer channel.stateMutex.RUnlock()
modes, present := channel.members[client]
if present {
for _, mode := range modes.AllModes() {
result = append(result, mode.String())
}
}
return
}

func (channel *Channel) ClientJoinTime(client *Client) time.Time {
channel.stateMutex.RLock()
defer channel.stateMutex.RUnlock()
time := channel.memberJoinTimes[client]
return time
}

func (channel *Channel) ClientHasPrivsOver(client *Client, target *Client) bool {
channel.stateMutex.RLock()
clientModes := channel.members[client]
Expand Down Expand Up @@ -707,6 +728,7 @@ func (channel *Channel) Join(client *Client, key string, isSajoin bool, rb *Resp
defer channel.stateMutex.Unlock()

channel.members.Add(client)
channel.memberJoinTimes[client] = time.Now().UTC()
firstJoin := len(channel.members) == 1
newChannel := firstJoin && channel.registeredFounder == ""
if newChannel {
Expand Down Expand Up @@ -1316,6 +1338,7 @@ func (channel *Channel) Quit(client *Client) {

channel.stateMutex.Lock()
channel.members.Remove(client)
delete(channel.memberJoinTimes, client)
channelEmpty := len(channel.members) == 0
channel.stateMutex.Unlock()
channel.regenerateMembersCache()
Expand Down
4 changes: 4 additions & 0 deletions irc/commands.go
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,10 @@ func init() {
minParams: 1,
oper: true,
},
"EXTJWT": {
handler: extjwtHandler,
minParams: 1,
},
"HELP": {
handler: helpHandler,
minParams: 0,
Expand Down
18 changes: 16 additions & 2 deletions irc/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -469,6 +469,11 @@ type TorListenersConfig struct {
MaxConnectionsPerDuration int `yaml:"max-connections-per-duration"`
}

type JwtServiceConfig struct {
ExpiryInSeconds int64 `yaml:"expiry-in-seconds"`
Secret string
DanielOaks marked this conversation as resolved.
Show resolved Hide resolved
}

// Config defines the overall configuration.
type Config struct {
Network struct {
Expand Down Expand Up @@ -496,8 +501,9 @@ type Config struct {
MOTDFormatting bool `yaml:"motd-formatting"`
ProxyAllowedFrom []string `yaml:"proxy-allowed-from"`
proxyAllowedFromNets []net.IPNet
WebIRC []webircConfig `yaml:"webirc"`
MaxSendQString string `yaml:"max-sendq"`
WebIRC []webircConfig `yaml:"webirc"`
JwtServices map[string]JwtServiceConfig `yaml:"jwt-services"`
MaxSendQString string `yaml:"max-sendq"`
MaxSendQBytes int
AllowPlaintextResume bool `yaml:"allow-plaintext-resume"`
Compatibility struct {
Expand Down Expand Up @@ -891,6 +897,13 @@ func LoadConfig(filename string) (config *Config, err error) {
config.Server.capValues[caps.Multiline] = multilineCapValue
}

// confirm jwt config
for name, info := range config.Server.JwtServices {
if info.Secret == "" {
return nil, fmt.Errorf("Could not parse jwt-services config, %s service has no secret set", name)
}
}

// handle legacy name 'bouncer' for 'multiclient' section:
if config.Accounts.Bouncer != nil {
config.Accounts.Multiclient = *config.Accounts.Bouncer
Expand Down Expand Up @@ -1135,6 +1148,7 @@ func (config *Config) generateISupport() (err error) {
isupport.Add("CHANTYPES", chanTypes)
isupport.Add("ELIST", "U")
isupport.Add("EXCEPTS", "")
isupport.Add("EXTJWT", "1")
isupport.Add("INVEX", "")
isupport.Add("KICKLEN", strconv.Itoa(config.Limits.KickLen))
isupport.Add("MAXLIST", fmt.Sprintf("beI:%s", strconv.Itoa(config.Limits.ChanListModes)))
Expand Down
76 changes: 76 additions & 0 deletions irc/handlers.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
"strings"
"time"

"github.com/dgrijalva/jwt-go"
"github.com/goshuirc/irc-go/ircfmt"
"github.com/goshuirc/irc-go/ircmatch"
"github.com/goshuirc/irc-go/ircmsg"
Expand Down Expand Up @@ -889,6 +890,81 @@ func dlineHandler(server *Server, client *Client, msg ircmsg.IrcMessage, rb *Res
return killClient
}

// EXTJWT <target> [service_name]
func extjwtHandler(server *Server, client *Client, msg ircmsg.IrcMessage, rb *ResponseBuffer) bool {
expireInSeconds := int64(30)

accountName := client.AccountName()
if accountName == "*" {
accountName = ""
}

claims := jwt.MapClaims{
"iss": server.name,
"sub": client.Nick(),
"account": accountName,
"umodes": []string{},
}

if msg.Params[0] != "*" {
channel := server.channels.Get(msg.Params[0])
if channel == nil {
rb.Add(nil, server.name, "FAIL", "EXTJWT", "NO_SUCH_CHANNEL", client.t("No such channel"))
return false
}

claims["channel"] = channel.Name()
claims["joined"] = 0
claims["cmodes"] = []string{}
if channel.hasClient(client) {
joinTime := channel.ClientJoinTime(client)
if joinTime.IsZero() {
// shouldn't happen, only in races
rb.Add(nil, server.name, "FAIL", "EXTJWT", "UNKNOWN_ERROR", client.t("Channel join time is inconsistent, JWT not generated"))
return false
}
claims["joined"] = joinTime.Unix()
claims["cmodes"] = channel.ClientModeStrings(client)
}
}

// we default to a secret of `*`. if you want a real secret setup a service in the config~
Copy link
Member

Choose a reason for hiding this comment

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

What's going on here exactly?

Copy link
Member Author

@DanielOaks DanielOaks Apr 17, 2020

Choose a reason for hiding this comment

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

If you provide a service name (like jitsi-call or acme-image-hosting) then it'll use the secret defined in that service's config block. By default it just uses a secret of * because there's really no reason to have a default secret that's customised (anything custom will just use a service name anyways).

Copy link
Member

Choose a reason for hiding this comment

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

Are we sure signing with a secret of * is correct? I found people saying that if there is no secret one should simply omit the signature: https://riptutorial.com/jwt/example/18558/unsigned-jwt

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, we should not be mixing and matching with alg: none because of the very frequent issues that alg:none has caused in the past. I'm not sure whether using * in particular is correct, but if the spec recommends using none rather than hmac then it is a horrendous decision honestly.

Copy link
Member

@slingamn slingamn Apr 19, 2020

Choose a reason for hiding this comment

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

I'm not 100% clear on what's going on here but I have a really strong intuition that using a secret key of * is incorrect.

If I'm reading the spec correctly, we should be requiring the operator to specify a default key of some kind (which should be a genuine secret and not *), and if they don't we should disable JWT (the command and the ISUPPORT token).

Should we ask for clarification on the spec PR?

Copy link
Member Author

@DanielOaks DanielOaks Apr 19, 2020

Choose a reason for hiding this comment

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

The spec should clarify this, yeah. There should not be any way to have a custom default secret, any custom secret should be provided by using a service name. So we should probably remove this and only advertise JWT if there is at least one service configured.

Copy link
Member

Choose a reason for hiding this comment

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

We got the clarification we wanted:

  1. The JWT header contains the signature algorithm, and the EXTJWT spec allows the use of any signature algorithm that can be verified. So I think what we have is fine. In the future, we could add, e.g., RSAPrivateKey to the JWTServiceConfig, and then the EXTJWT handler could sign with RSA instead if that field is populated.
  2. We should in fact have a default secret (and EXTJWT should be disabled if one is not specified).

Copy link
Member Author

@DanielOaks DanielOaks Apr 19, 2020

Choose a reason for hiding this comment

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

  1. So long as we never sign anything using the None algorithm, cool. Yeah could definitely add that key entry to the config in future no worries.
  2. But that... doesn't answer our question. Should the default be static (same on all nets, same everywhere, specified in the specification, etc) or should the default be decided by the network and it's totally OK if clients can't actually verify it? Prawn's response indicates that this is intended for use by clients that don't actually know anything about the ircd setup as well so would having a configurable default actually make sense there or would it e.g. screw up clients trying to just decode data (for example do any client libraries out there intentionally not decode stuff if the user doesn't have the correct verification key?)

Copy link
Member

Choose a reason for hiding this comment

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

should the default be decided by the network and it's totally OK if clients can't actually verify it

I'm very confident that this is the correct answer. Clients do not parse/validate the JWT token themselves, they hand it off to a third party. That third party is assumed to hold the correct verification key. (The original use case for this is a closed ecosystem of an ircd and a Jitsi, controlled by the same operator.)

A "secret key" that is a constant value written into a specification just doesn't make sense.

service := "*"
secret := "*"
if 1 < len(msg.Params) {
service = strings.ToLower(msg.Params[1])

c := server.Config()
info, exists := c.Server.JwtServices[service]
if !exists {
rb.Add(nil, server.name, "FAIL", "EXTJWT", "NO_SUCH_SERVICE", client.t("No such service"))
return false
}
secret = info.Secret
if info.ExpiryInSeconds != 0 {
expireInSeconds = info.ExpiryInSeconds
}
}
claims["exp"] = time.Now().Unix() + expireInSeconds

token := jwt.NewWithClaims(jwt.SigningMethodHS256, claims)
tokenString, err := token.SignedString([]byte(secret))

if err == nil {
maxTokenLength := 400

for maxTokenLength < len(tokenString) {
Copy link
Member

Choose a reason for hiding this comment

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

very elegant, i like this

rb.Add(nil, server.name, "EXTJWT", msg.Params[0], service, "*", tokenString[:maxTokenLength])
tokenString = tokenString[maxTokenLength:]
}
rb.Add(nil, server.name, "EXTJWT", msg.Params[0], service, tokenString)
} else {
rb.Add(nil, server.name, "FAIL", "EXTJWT", "UNKNOWN_ERROR", client.t("Could not generate EXTJWT token"))
}

return false
}

// HELP [<query>]
func helpHandler(server *Server, client *Client, msg ircmsg.IrcMessage, rb *ResponseBuffer) bool {
argument := strings.ToLower(strings.TrimSpace(strings.Join(msg.Params, " ")))
Expand Down
5 changes: 5 additions & 0 deletions irc/help.go
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,11 @@ ON <server> specifies that the ban is to be set on that specific server.
[reason] and [oper reason], if they exist, are separated by a vertical bar (|).

If "DLINE LIST" is sent, the server sends back a list of our current DLINEs.`,
},
"extjwt": {
text: `EXTJWT <target> [service_name]

Get a JSON Web Token for target (either * or a channel name).`,
},
"help": {
text: `HELP <argument>
Expand Down
11 changes: 11 additions & 0 deletions oragono.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,17 @@ server:
# - "192.168.1.1"
# - "192.168.10.1/24"

# these services can integrate with the ircd using JSON Web Tokens (https://jwt.io)
# sometimes referred to with 'EXTJWT'
jwt-services:
# # service name
# call-host:
# # custom expiry length, default is 30s
# expiry-in-seconds: 45

# # secret string to verify the generated tokens
# secret: call-hosting-secret-token

# allow use of the RESUME extension over plaintext connections:
# do not enable this unless the ircd is only accessible over internal networks
allow-plaintext-resume: false
Expand Down
4 changes: 4 additions & 0 deletions vendor/github.com/dgrijalva/jwt-go/.gitignore

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

13 changes: 13 additions & 0 deletions vendor/github.com/dgrijalva/jwt-go/.travis.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 8 additions & 0 deletions vendor/github.com/dgrijalva/jwt-go/LICENSE

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading