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

Allow using a custom XMPP server #12

Closed
mkai opened this issue Apr 7, 2012 · 22 comments
Closed

Allow using a custom XMPP server #12

mkai opened this issue Apr 7, 2012 · 22 comments
Milestone

Comments

@mkai
Copy link

mkai commented Apr 7, 2012

Thanks for your effort!

I'm using my own XMPP server (ejabberd) - do you think adding support for using arbitrary XMPP servers besides Google's would be a major undertaking?

@chrisballinger
Copy link
Member

It shouldn't be, I think I tested a jabber.org login and it worked, but I'll have to look into it more. XMPPFramework supports custom servers to my knowledge and if I recall correctly I even enabled the option to accept self signed SSL Certs.

@vitalyster
Copy link
Contributor

Already works with custom server

@chrisballinger
Copy link
Member

@mkai have you been able to get it working with a custom server? You should be able to just type in account@example.com for the username field. Not sure whether I should mark this issue as closed or not because vitalyster says he has had success with his custom server.

@vitalyster
Copy link
Contributor

I'm not familiar neither Off-the-Record, nor xmppframework code, but proper xmpp client should get hostname to connect via dns SRV query - http://tools.ietf.org/html/rfc6120#section-3.2.1
I'm using xmpp.ru, which hostname equals to domain name, so I'm able to connect, but if hostname and domain are differ - it may fail.

@chrisballinger
Copy link
Member

I think XMPPFramework only works with hostname/domain differences for Google Talk via a hardcoded function, so that could explain the problem.

@mkai
Copy link
Author

mkai commented Apr 12, 2012

I've set up my XMPP server with the relevant SRV DNS entries, so the resolving is not the problem. Rather it fails because I'm using a self-signed SSL cert. I can only connect if I change in OTRXMPPManager.m:

-       allowSelfSignedCertificates = NO;
-       allowSSLHostNameMismatch = NO;
+       allowSelfSignedCertificates = YES;
+       allowSSLHostNameMismatch = YES;

OTR key exchange and chatting works nicely, then. Only problem is the SSL cert.

@chrisballinger
Copy link
Member

I have been working on a new Settings view and will now plan to make these options user-configurable in the next release.

@mkai
Copy link
Author

mkai commented Apr 12, 2012

EDIT: I wrote that before your answer came in - great news! Thanks!

I guess just changing that in upstream would compromise security for Google Talk users for no good reason - so I'd suggest adding a "Custom" tab to add a custom XMPP server, with a setting to allow self-signed SSL certs. My ObjC skills are very rusty, though. So I'll leave it up to you depending on if you think it would benefit the app.

@chrisballinger
Copy link
Member

Good call. I was going to overhaul the way accounts were added as well and,
you're right, it seems better to allow on a per-server basis than on a
global level.

On Thu, Apr 12, 2012 at 12:43 PM, Markus Kaiserswerth <
reply@reply.github.com

wrote:

I guess just changing that in upstream would compromise security for
Google Talk users for no good reason - so I'd suggest adding a "Custom" tab
to add a custom XMPP server, with a setting to allow self-signed SSL certs.
My ObjC skills are very rusty, though. So I'll leave it up to you depending
on if you think it would benefit the app.


Reply to this email directly or view it on GitHub:

#12 (comment)

@chrisballinger
Copy link
Member

@mkai: I added a rudimentary UI for changing those settings in this commit: c25b03c

Let me know what you think!

@mkai
Copy link
Author

mkai commented Apr 13, 2012

Hi Chris,

I need to disable ARC in the project's settings to be able to compile OTRSettingsManager.m and OTRSetting.m:

Off-the-Record-iOS/Off the Record/OTRSettingsManager.m:19:13: error: ARC forbids synthesizing a property of an Objective-C object with unspecified ownership or storage attribute [4]
Off-the-Record-iOS/Off the Record/OTRSettingsManager.m:31:10: error: receiver type 'OTRSettingsManager' for instance message does not declare a method with selector 'populateSettings' [4]
Off-the-Record-iOS/Off the Record/OTRSetting.m:12:13: error: ARC forbids synthesizing a property of an Objective-C object with unspecified ownership or storage attribute [4]
Off-the-Record-iOS/Off the Record/OTRSetting.m:12:20: error: ARC forbids synthesizing a property of an Objective-C object with unspecified ownership or storage attribute [4]

If I disable ARC, I get the following exception when I tap "Settings" in the simulator:

2012-04-13 13:37:55.304 ChatSecure[7463:fb03] -[__NSMallocBlock__ count]: unrecognized selector sent to instance 0x7995ad0
2012-04-13 13:37:55.306 ChatSecure[7463:fb03] *** Terminating app due to uncaught exception 'NSInvalidArgumentException', reason: '-[__NSMallocBlock__ count]: unrecognized selector sent to instance 0x7995ad0'

On a similar note, I also got this error when compiling (solved it for testing by uncommenting the line until now):

Off-the-Record-iOS/Off the Record/OTRChatViewController.m:302:6: error: receiver type 'OTRChatViewController' for instance message does not declare a method with selector 'refreshView' [4]

Using Xcode Version 4.2.1

@chrisballinger
Copy link
Member

Try updating Xcode to 4.3.2 (only available for 10.7+ and through the Mac App Store), but I'll see if I can fix it for you.

@chrisballinger
Copy link
Member

See if this fixes your issue: 1fa3d3f

@mkai
Copy link
Author

mkai commented Apr 13, 2012

4.2.1 still complained about a missing storage attribute on some properties, so I googled a bit and did the following changes in order to compile ('strong' is nothing but a guess, so I'm not really up to sending a real pull request - hope it's helpful anyway):

diff --git a/Off the Record/OTRSettingsGroup.h b/Off the Record/OTRSettingsGroup.h
index ed2f0c6..42cc7ad 100644
--- a/Off the Record/OTRSettingsGroup.h 
+++ b/Off the Record/OTRSettingsGroup.h 
@@ -10,8 +10,8 @@

 @interface OTRSettingsGroup : NSObject

-@property (nonatomic, readonly) NSArray *settings;
-@property (nonatomic, readonly) NSString *title;
+@property (strong, nonatomic, readonly) NSArray *settings;
+@property (strong, nonatomic, readonly) NSString *title;

 - (id) initWithTitle:(NSString*)newTitle settings:(NSArray*)newSettings;

diff --git a/Off the Record/OTRValueSetting.h b/Off the Record/OTRValueSetting.h
index ac07280..599d94e 100644
--- a/Off the Record/OTRValueSetting.h  
+++ b/Off the Record/OTRValueSetting.h  
@@ -10,7 +10,7 @@

 @interface OTRValueSetting : OTRSetting

-@property (nonatomic, readonly) NSString *key;
+@property (strong, nonatomic, readonly) NSString *key;
 @property (nonatomic) id value;

 - (id) initWithTitle:(NSString*)newTitle description:(NSString*)newDescription settingsKey:(NSString*)newSettingsKey;
diff --git a/Off the Record/OTRViewSetting.h b/Off the Record/OTRViewSetting.h
index 99c61a5..3dd73f4 100644
--- a/Off the Record/OTRViewSetting.h   
+++ b/Off the Record/OTRViewSetting.h   
@@ -15,7 +15,7 @@

 @interface OTRViewSetting : OTRSetting

-@property (nonatomic, readonly) Class viewControllerClass;
+@property (strong, nonatomic, readonly) Class viewControllerClass;
 @property (nonatomic, retain) id<OTRViewSettingDelegate> delegate;

@chrisballinger
Copy link
Member

Yeah, I need to read a little bit more about ARC conventions, but it seems
like that is the right thing to do. I don't know why Apple relaxed the
compiler errors and warnings in 4.3.2, it would have been nice to catch
these problems in the later version as well. From what I've read "strong"
is essentially equivalent to "retain".

On Fri, Apr 13, 2012 at 10:14 AM, Markus Kaiserswerth <
reply@reply.github.com

wrote:

4.2.1 still complained about a missing storage attribute on some
properties, so I googled a bit and did the following changes in order to
compile ('strong' is nothing but a guess, so I'm not really up to sending a
real pull request - hope it's helpful anyway):

diff --git a/Off the Record/OTRSettingsGroup.h b/Off the
Record/OTRSettingsGroup.h
index ed2f0c6..42cc7ad 100644
--- a/Off the Record/OTRSettingsGroup.h
+++ b/Off the Record/OTRSettingsGroup.h
@@ -10,8 +10,8 @@

 @interface OTRSettingsGroup : NSObject

-@property (nonatomic, readonly) NSArray *settings;
-@property (nonatomic, readonly) NSString *title;
+@property (strong, nonatomic, readonly) NSArray *settings;
+@property (strong, nonatomic, readonly) NSString *title;

 - (id) initWithTitle:(NSString*)newTitle settings:(NSArray*)newSettings;

diff --git a/Off the Record/OTRValueSetting.h b/Off the
Record/OTRValueSetting.h
index ac07280..599d94e 100644
--- a/Off the Record/OTRValueSetting.h
+++ b/Off the Record/OTRValueSetting.h
@@ -10,7 +10,7 @@

 @interface OTRValueSetting : OTRSetting

-@property (nonatomic, readonly) NSString *key;
+@property (strong, nonatomic, readonly) NSString *key;
 @property (nonatomic) id value;

 - (id) initWithTitle:(NSString*)newTitle
description:(NSString*)newDescription settingsKey:(NSString*)newSettingsKey;
diff --git a/Off the Record/OTRViewSetting.h b/Off the
Record/OTRViewSetting.h
index 99c61a5..3dd73f4 100644
--- a/Off the Record/OTRViewSetting.h
+++ b/Off the Record/OTRViewSetting.h
@@ -15,7 +15,7 @@

 @interface OTRViewSetting : OTRSetting

-@property (nonatomic, readonly) Class viewControllerClass;
+@property (strong, nonatomic, readonly) Class viewControllerClass;
 @property (nonatomic, retain) id<OTRViewSettingDelegate> delegate;

Reply to this email directly or view it on GitHub:

#12 (comment)

@mkai
Copy link
Author

mkai commented Apr 13, 2012

As for the runtime behaviour: it works and I can connect using the new options!

There's a small bug, however: if I enable the option and then try to connect directly afterwards, it doesn't work. I have to restart the app in order for it to pickup the setting. I guess the setting's just not updated or not read from UserDefaults.

After that initial hiccup, it always works.

@mkai
Copy link
Author

mkai commented Apr 13, 2012

And please don't bother spending time on supporting 4.2.1 on my account :) I've just upgraded to 4.3.2.

@chrisballinger
Copy link
Member

Oh sweet! I'm glad it works for you. It looks like I forgot to "synchronize" the NSUserDefaults somewhere, I'll look into that. Thanks!

@chrisballinger
Copy link
Member

Ok I'm gonna close this issue, feel free to reopen it if you have any other problems. Thanks!

@epitron
Copy link

epitron commented Jul 30, 2012

Just a small point on security. Once multiple xmpp accounts are supported, self-signing needs to be a per-account setting, otherwise services like gtalk will be open to man-in-the-middle attacks.

Also important is to notify the user whenever the self-signed certificate changes - again, to make MitM attacks visible.

@epitron
Copy link

epitron commented Jul 30, 2012

(mitm attacks are much more likely on mobile devices, which connect to a lot of open wifi networks. :)

@chrisballinger
Copy link
Member

This is a very good catch. I'll make it a priority to make those options per-account. I'll see if I can implement a notification when the self-signed certificate changes as well, although this might require some hacking on XMPPFramework.

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

No branches or pull requests

4 participants